Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FR] Optimize XmlDependencies.IsDependenciesFile for Performance #601

Open
chrisyarbrough opened this issue Mar 15, 2023 · 3 comments
Open

Comments

@chrisyarbrough
Copy link

chrisyarbrough commented Mar 15, 2023

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!

@google-oss-bot
Copy link

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

@paulinon paulinon removed the new to be triaged label Mar 16, 2023
chrisyarbrough pushed a commit to chrisyarbrough/unity-jar-resolver that referenced this issue Mar 17, 2023
This simpler string manipulation checks the same logic as before but much faster in the context of a large project. If the OnPostprocessAllAssets callback is invoked multiple times with thousands of files, as can happen with non-trivial Unity projects, the regex call becomes quite expensive.

For profiling data see: googlesamples#601
@chkuang-g
Copy link
Collaborator

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
Copy link
Author

Thanks for your feedback, Shawn! :) I've opened a PR: #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 pushed a commit to chrisyarbrough/unity-jar-resolver that referenced this issue Feb 9, 2024
This simpler string manipulation checks the same logic as before but much faster in the context of a large project. If the OnPostprocessAllAssets callback is invoked multiple times with thousands of files, as can happen with non-trivial Unity projects, the regex call becomes quite expensive.

For profiling data see: googlesamples#601
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants