googlesamples / unity-jar-resolver

Unity plugin which resolves Android & iOS dependencies and performs version management
Other
1.21k stars 337 forks source link

[FR] Optimize XmlDependencies.IsDependenciesFile for Performance #601

Open chrisyarbrough opened 1 year ago

chrisyarbrough commented 1 year ago

Feature proposal

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:

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

google-oss-bot commented 1 year ago

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

chkuang-g commented 1 year ago

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

chrisyarbrough commented 1 year ago

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