unity-jar-resolver icon indicating copy to clipboard operation
unity-jar-resolver copied to clipboard

[FR] Optimize XmlDependencies.IsDependenciesFile for Performance

Open chrisyarbrough opened this issue 2 years ago • 3 comments

Feature proposal

  • EDM4U Component: Android Resolver, iOS Resolver

I've observed a significant performance slowdown in the Unity Editor when using Android Resolver and iOS Resolver in a large project with numerous assets:

XmlDependencies_IsDependenciesFile_Performance

As seen in the profiler screenshot, the XmlDependencies.IsDependenciesFile method takes 30 seconds to execute in my current project setup. While reducing the calls to this method by refreshing the Unity AssetDatabase less frequently could be a solution, it may not always be feasible due to certain project limitations. Instead, optimizing this method seems more practical.

The performance bottleneck appears to be the regex pattern in the following code snippet:

internal HashSet<Regex> fileRegularExpressions = new HashSet<Regex> {
    new Regex(@".*[/\\]Editor[/\\].*Dependencies\.xml$")
};

internal bool IsDependenciesFile(string filename) {
    foreach (var regex in fileRegularExpressions) {
        if (regex.Match(filename).Success) {
            return true;
        }
    }
    return false;
}

A possible optimization involves replacing the regex pattern with simpler string manipulation methods like this example (untested):

internal bool IsDependenciesFile(string filename) {
    const string EditorFolder = "/Editor/";
    const string DependenciesFileSuffix = "Dependencies.xml";
    int editorIndex = filename.IndexOf(EditorFolder, StringComparison.OrdinalIgnoreCase);
    
    return editorIndex >= 0 && filename.EndsWith(DependenciesFileSuffix, StringComparison.OrdinalIgnoreCase);
}

However, PlayServicesResolver also uses fileRegularExpressions to add additional patterns. I'm unsure how this can be easily accommodated, but I propose the following potential improvements:

  • Replace PlayServicesResolver patterns with simpler string methods, if possible
  • Change fileRegularExpressions from a collection of Regex objects to a delegate or filter object that can be either a Regex or a simpler string manipulation
  • Examine the regex pattern and options for possible enhancements

I would be happy to investigate this issue myself and test changes in my project. Any feedback is greatly appreciated!

chrisyarbrough avatar Mar 15 '23 22:03 chrisyarbrough

This issue does not seem to follow the issue template. Make sure you provide all the required information.

google-oss-bot avatar Mar 15 '23 22:03 google-oss-bot

I see your point.

I think the key issue is that iOS Resolver is trying to scan through every changed asset and run a regex against their path. This is definitely not good if you have a large project and modify many files at the same time.

https://github.com/googlesamples/unity-jar-resolver/blob/master/source/IOSResolver/src/IOSResolver.cs#L1941

You code actually changed the logic: The current logic is looking for any *Dependencies.xml file under a folder named Editor. Your code will change to look for any *Dependencies.xml file under any subfolder of a folder named Editor.

Since the searching so relatively simple, I think it can do

  1. Check if the path ends with Dependencies.xml. If not, return false. This is a 16 characters comparison per path and should filter out majority of the paths.
  2. Check if the parent folder is Editor. I expect there is not that lot of Dependencies.xml in any given project. At this point, either regex or path parsing would matter that much.

We are currently working on other EDM4U issue. If you like to, I would encourage you to send us an PR.

Shawn

chkuang-g avatar Mar 20 '23 21:03 chkuang-g

Thanks for your feedback, Shawn! :) I've opened a PR: https://github.com/googlesamples/unity-jar-resolver/pull/602

Sorry about the code sample, I didn't think it through or test it, but in my PR I created a test class to make sure the updated logic is the same as before and also tested in the production project in which my company is using the Google plugin.

image

I've inserted the changes so that new public API stays compatible with previous versions for now. I might need some help with releasing the correct build artifacts though, not sure which one I need to commit.

Best, Chris

chrisyarbrough avatar Mar 21 '23 13:03 chrisyarbrough