1
\$\begingroup\$

I'm wondering how I could optimize this method that takes in a list of UserPrincipal objects, filters them by Regex and inserts the new class instance into a list. It takes 4 to 5 seconds for an input list of 2000 entries, which seems too slow for that amount of entries.

private static Dictionary<string, User> ProcessUsers(IEnumerable<UserPrincipal> users) { Dictionary<string, User> toReturn= new Dictionary<string, User>(); Regex regex = new Regex(@"^[a-zA-Z]{3}(\d{4})$"); foreach (UserPrincipal user in users) { if (regex.IsMatch(user.SamAccountName)) { User newUser = new User(user.Name, user.SamAccountName.ToUpper(), user.EmailAddress, user); toReturn.TryAdd(newUser.ID, newUser); } } return toReturn; } 

I've tried Parallel.ForEach, but it didn't help at all.

EDIT: User class properties and constructor:

internal class User { public string name { get; set; } public string ID { get; set; } public string email { get; set; } public UserPrincipal? activeDirectoryHandle { get; set; } public User(string inName = "None", string inID = "None", string inEmail = "None", UserPrincipal? adHandle=null) { name = inName; ID = inID; email = inEmail; activeDirectoryHandle = adHandle; } } 
\$\endgroup\$
6
  • \$\begingroup\$Welcome to CodeReview! Have you tried to run some profiler, like CodeTrack?\$\endgroup\$CommentedOct 27, 2022 at 9:50
  • \$\begingroup\$Also can you please share with us the definition of the User class?\$\endgroup\$CommentedOct 27, 2022 at 9:51
  • 1
    \$\begingroup\$@PeterCsala Added the User class to the question. I'll check out CodeTrack, I've never worked with a profiler before, I'm still relatively green\$\endgroup\$
    – mvi2110
    CommentedOct 27, 2022 at 10:37
  • \$\begingroup\$Is it possible that your users collection contains multiple UserPrincipals where the SamAccountName is the same?\$\endgroup\$CommentedOct 27, 2022 at 10:58
  • 1
    \$\begingroup\$have you tried using RegexOptions.Compiled ?\$\endgroup\$
    – iSR5
    CommentedOct 27, 2022 at 12:58

2 Answers 2

1
\$\begingroup\$

In this, as in many other performance-related issues you should use a profiler (or at least time the execution yourself).

There is nothing inherently slow in your code, however I can think of several things that can potentially be causing the issue:

  • IEnumerable is a useful abstraction, however it also means that any amount of work can be hidden by it. It is possible, that the data is being loaded lazily from a slow data source at the point of enumeration.
  • SamAccountName is also not as simple as it looks. If you look at the source code, you'll see that it is not just a string return operation, it is quite elaborate with lazy loading of the data, which might be slow depending on the source from where it is gathered from
  • It should not happen if those are normal user principals, but for the sake of completeness, if all the user IDs are the same, inserting into dictionary might be not particularly fast either.
\$\endgroup\$
1
  • \$\begingroup\$I changed the IEnumarble to List<Users> and it did indeed cut down the average run time of the function by about 1.5 to 2 seconds on average. I didn't know it can affect speed like that. SamAccountName I assumed to be just a string as that's what it shows as in the debugger when I examine the UserPrincipal object. Just to be clear, there is no data gathering going on at this point in the application. Everything is working on in-memory data. Do you mean if any two user IDs are the same? It's only possible if someone with AD access changed it manually after the fact to an existing one.\$\endgroup\$
    – mvi2110
    CommentedOct 28, 2022 at 5:53
0
\$\begingroup\$

If the source collection does not contain multiple UserPrincipals where the SamAccountName is the same then you can do that same with Linq:

private static Regex regex = new Regex(@"^[a-zA-Z]{3}(\d{4})$", RegexOptions.Compiled); private static Dictionary<string, User> ProcessUsers(IEnumerable<UserPrincipal> users) => users .Where(user => regex.IsMatch(user.SamAccountName)) .Select(user => new User(user.Name, user.SamAccountName.ToUpper(), user.EmailAddress, user)) .ToDictionary(u => u.ID, u => u); 

I don't think it will perform better but it is more declarative and more concise.

\$\endgroup\$

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.