Optimize search for the default bundle

[jiahaog]

+[NSBundle bundleWithIdentifier] is slow, and scales to the size of the app. This call during startup significantly affects the startup of customer: chalk.

This implementation searches within the privateFrameworksURL where the default bundle is expected to be located. When tested internally, this improves startup of customer: chalk by 30% - see http://shortn/_WcifhO48X6.

It also falls back to +[NSBundle bundleWithIdentifier] so there is no functional change in behavior for customers that may be using alternate build systems.

List which issues are fixed by this PR. You must list at least one issue.

Fix flutter/flutter#37826

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[jmagman]

When tested internally, this improves startup of customer: chalk by 30% - see http://shortn/_WcifhO48X6.

Wow, that's significant!

1. Does macOS have the same problem?
   

   engine/shell/platform/darwin/macos/framework/Source/FlutterDartProject.mm

   Line 29 in 706fd9c

   _dartBundle = bundle ?: [NSBundle bundleWithIdentifier:kAppBundleIdentifier];

2. Can you reuse this in other places in FlutterDartProject, see for example
   

   engine/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm

   Lines 111 to 112 in 706fd9c

   	NSString* applicationFrameworkPath = [mainBundle pathForResource:@"Frameworks/App.framework"
   	ofType:@""];

   
   

   engine/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm

   Line 312 in 706fd9c

   bundle = [NSBundle bundleWithIdentifier:[FlutterDartProject defaultBundleIdentifier]];

3. Testing

As this is a pure performance change, no tests are added.

I don't think this is strictly true since it's adding lines of code that sure could have a bug in them, causing it to always incorrectly fall back to the slow implementation...
You could make FLTBundleWithIdentifier an extern in FlutterDartProject_Internal.h and test it explicitly. For example, you could create all the files needed to create a NSBundle in the test, pass the URL hint in, and then make sure that one is selected.

[jmagman]

I don't think this needs to be static, you can nudge the compiler to inline with NS_INLINE.

[jiahaog]

Done!

Also regarding the general review comment:

You could make FLTBundleWithIdentifier an extern in FlutterDartProject_Internal.h and test it explicitly.

This is pretty new to me, but it does seem like using an extern is incompatible with NS_INLINE, or static methods in general. I couldn't make FLTFrameworkBundleInternal static / extern, if we want to export it in FlutterDartProject_Internal.h. Please let me know if you have any ideas or if this is ok.

[jmagman]

Instead of contentsOfDirectoryAtURL, which loads all the contents of a directory into an array even if you only were looking for the first one, there's a NSDirectoryEnumerator which will walk the tree as you enumerate over it (maybe overkill for this use-case with a small number of files, but since you're optimizing), you can tell it to skip hidden files, do a shallow traversal, etc:

NS_INLINE NSBundle* FLTBundleWithIdentifier(NSString* bundleID, NSURL* hintURL) {
  NSDirectoryEnumerator<NSURL *> *frameworkEnumerator = [NSFileManager.defaultManager
                                         enumeratorAtURL:hintURL
                              includingPropertiesForKeys:nil
                                                 options:NSDirectoryEnumerationSkipsSubdirectoryDescendants|NSDirectoryEnumerationSkipsHiddenFiles
                                            // Not interested in the error as there is a fallback.
                                            errorHandler:nil];
  for (NSURL* candidate in frameworkEnumerator) {
    NSBundle* bundle = [NSBundle bundleWithURL:candidate];
    if ([bundle.bundleIdentifier isEqualToString:bundleID]) {
      return bundle;
    }
  }

  // Fallback to slow implementation.
  return [NSBundle bundleWithIdentifier:bundleID];
}

[jiahaog]

I didn't know about this, thanks for the suggestion, done!

[jmagman]

minor nit, URLByDeletingLastPathComponent is a property, dot notation:

Suggested change

	NSURL* defaultBundleHintURL = [engineBundle.bundleURL URLByDeletingLastPathComponent];
	NSURL* defaultBundleHintURL = engineBundle.bundleURL.URLByDeletingLastPathComponent;

[jiahaog]

This is obsolete - see the next comment below

[jmagman]

If this is still nil you could call it again with the hint NSBundle.mainBundle.privateFrameworksURL (the main bundle Frameworks directory), not sure if that case is actually likely though.

[jiahaog]

Using this is actually much better and keeps the code simpler. It turns out that NSBundle.mainBundle.privateFrameworksURL resolves to the same thing as engineBundle.bundleURL.URLByDeletingLastPathComponent. As such, I removed the need to pass a hint and renamed FLTBundleWithIdentifier to FLTFrameworkBundleWithIdentifier and always search within this path.

[jiahaog]

Thank you for the in-depth review!

1. Does macOS have the same problem?
   

   engine/shell/platform/darwin/macos/framework/Source/FlutterDartProject.mm

   Line 29 in 706fd9c

   _dartBundle = bundle ?: [NSBundle bundleWithIdentifier:kAppBundleIdentifier];

I don't have a setup for profiling macOS apps now so I can't say for sure now, but I would expect this to also happen there – we should perhaps follow up separately. It seems like Chrome also encountered this problem on macOS - see GetFrameworkBundlePath (L156) of https://chromium.googlesource.com/chromium/src/+/master/chrome/common/chrome_paths_mac.mm.

2. Can you reuse this in other places in FlutterDartProject, see for example
   

   engine/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm

   Lines 111 to 112 in 706fd9c

   	NSString* applicationFrameworkPath = [mainBundle pathForResource:@"Frameworks/App.framework"
   	ofType:@""];

We don't have the source to +[NSBundle bundleWithIdentifier:] but at least empirically through profiling and experimentation, this method is slow because it has to iterate through all the known bundles and read the bundle IDs from the plist files. Since the bundle and path is already known here, it shouldn't be a problem here.

engine/shell/platform/darwin/ios/framework/Source/FlutterDartProject.mm

Line 312 in 706fd9c

bundle = [NSBundle bundleWithIdentifier:[FlutterDartProject defaultBundleIdentifier]];

This is the last remaining reference to +[NSBundle bundleWithIdentifier:] on iOS, which I updated to use this helper as well.

You could make FLTBundleWithIdentifier an extern in FlutterDartProject_Internal.h and test it explicitly. For example, you could create all the files needed to create a NSBundle in the test, pass the URL hint in, and then make sure that one is selected.

I found it too hard to create a NSBundle in a test as this seems to be something that is created as part of the build process. Instead, I opted to try and find a known bundle that must / must not exist. I had to also factor out the implementation to not include the fallback to unit test the enumeration behavior specifically. Please take a look at the new changes in FlutterDartProjectTest.mm :)

[jmagman]

LGTM, thanks for tracking this weird perf issue down.

Filed flutter/flutter#122028 to track sharing the optimization on macOS.