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

Improve performance of XmlDependencies IsDependenciesFile #602

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

chrisyarbrough
Copy link

@chrisyarbrough chrisyarbrough commented Mar 17, 2023

Why this change?

This implements the improvement for issue: #601

The XmlDependencies.IsDependenciesFile method is called thousands of times in large projects and is now much faster than before:

IsDependenciesFile_Before

IsDependenciesFile _After

The previous slowdown was causing long waiting times during content iteration in Unity projects with more than a few hundred assets.

What did I do?

I added a new test project to add a test which fixates the previous logic of file pattern matches with regex. I then replaced the regex with simpler string manipulation.

To avoid breaking API changes, I kept the public method in PlayServicesResolver the same, but internally, the new code uses a delegate which accepts a file path and returns true if it's a match. This way we can have both the Regex expressions passed from existing user code and a faster version for the internal default which only searches for the Dependencies.xml files in editor folders.

Review & Release

The review should be easiest commit-by-commit.

Feedback is greatly appreciated and please let me know which steps I need to take to make my change release-ready or if any of the maintainers will take over. Thank you! :)

Tasks

  • Implement and unit-test my change
  • Test the change in a production project
  • Update the changelog in the repo root
  • Update the version in the build.gradle
  • Wait for PR feedback
  • Build the release using ./gradlew release
  • Create release commit using ./gradlew gitCreateReleaseCommit
  • Tag the release using ./gradlew gitTagReleaseand git push --tag REMOTE HEAD:master

CHANGELOG.md Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this.

Please don't make a release for us. 😆
We have an internal pipeline to release EDM4U and make sure the release is available for Github, Google APIs for Unity and every other Google products which is using EDM4U.

build.gradle Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this.

Same. Please don't make a release for us. :)

return false;
internal static bool IsDependenciesFile(string filename) {
bool isInEditorFolder = filename.Contains("/Editor/") || filename.Contains(@"\Editor\");
return isInEditorFolder && filename.EndsWith("Dependencies.xml");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is string parsing for 3 times (worst cast)
Also, I expect there are far more files to be under Editor folder than a file name ended with Dependencies.xml
Here is my recommendation:

  1. Early out when the filename does not end with Dependencies.xml
  2. Otherwise, use regex to check if the file is under Editor folder. sometime like ".*[/\\]Editor[/\\].*"

Regex is suppose to match at O(n) time so that should be better than parsing strings twice just to know if it is under Editor folder.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the early-out definitely makes it even faster.

However, as for the regex, that's the point of the PR: while profiling my large company project I noticed that the regex was taking considerable time compared to the more simple string comparisons. In theory, they should be the same, but the practical overhead appears to be quite substantial, even if pre-compiling the regex instruction.

@@ -0,0 +1,3 @@
namespace GooglePlayServices {
public delegate bool FileMatchPattern(string filename);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure the benefit to introduce this delegate and the change in PlayServicesResolver.cs. I do not think there would be any performance gain, is there?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, removed it!

Copy link
Collaborator

@chkuang-g chkuang-g Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kudos to you to add a Unit test for this!

Could you move the entire test to source/AndroidResolver/unit_tests for consistency sake. Thank you!

Also, please add the following lines to build.gradle, like right...here.

// TODO: Build and run NUnit tests for #602

Currently there are some issue with this build script to run NUnit tests from Unity 2019. While we do not want to burden you with the effort to fix it, adding a line here as a reminder would be nice.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this to source/AndroidResolver/unit_tests/src folder for consistency sake.

<Reference Include="System.Data" />
<Reference Include="System.Xml" />
<Reference Include="nunit.framework, Version=3.5.0.0, Culture=neutral, PublicKeyToken=2638cd05610744eb">
<HintPath>..\packages\NUnit.3.5.0\lib\net45\nunit.framework.dll</HintPath>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite specify about where you find NUnit library.
Which Unity version are you using? Is this one of the package downloaded from Unity Package Manager?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I copied this from a previous version of JarResolverTests, basically just added a nuget package reference to NUnit to run the tests in the Rider IDE outside of Unity. I'm not sure what the "packages" directory here is, but removing the HintPath doesn't prevent me from running the tests.

Please advise if there's a way to run the test that's better aligned with the rest of the project. My local project on macOS in Rider doesn't really compile all too well, and I'm used to much newer SDK versions of dotnet, so I'm not sure what the best way would be in this project. :)

@Tommigun1980
Copy link

@chkuang-g Hi. What's the status of this? When will it be released?

@Tommigun1980
Copy link

Hi @chkuang-g, when do you estimate this fix to land? Thanks.

@Tommigun1980
Copy link

@chkuang-g Hi, when will this PR land?

chrisyarbrough pushed a commit to chrisyarbrough/unity-jar-resolver that referenced this pull request Feb 9, 2024
Christopher Yarbrough added 8 commits February 9, 2024 09:33
This commit records the current behaviour of the method before I make changes to try and optimize it.
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
Internally, only Dependencies.xml files are matched, so we can use the more performant file-matching pattern here.

The public API remains unchanged and only supports regex for now.
This was only added because I initially thought it would read better, but it also obscures the in and out types. Since there's no functional difference, I removed it again just to reduce the amount of custom code.
@chrisyarbrough chrisyarbrough force-pushed the improve-performance-of-xmldependencies-isdependenciesfile branch from bdd2e8e to 167a3ce Compare February 9, 2024 08:34
@chrisyarbrough
Copy link
Author

I finally got some time to apply the review feedback. I retested the changes in my large company project and the custom changes still show some good improvements in Unity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants