Started looking for the dart plugin registrant at runtime by gaaclarke · Pull Request #31418 · flutter/engine

gaaclarke · 2022-02-12T00:24:38Z

Previously we'd shadow the main entrypoint which didn't work in release builds. Now we look for the registrant's presence at runtime to execute.

Companion framework PR: flutter/flutter#98046

The test for this is an integration tests that happens in the framework PR.

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.

flutter-dashboard · 2022-02-14T18:54:14Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

gaaclarke · 2022-02-22T19:35:43Z

@blasten friendly ping

blasten · 2022-02-25T01:01:21Z

runtime/dart_isolate.cc

+}
+
+Dart_Handle FindDartPluginRegistrantLibrary() {
+  // TODO(): Instead of finding this, it could be passed down from the tool.


do you plan to work on this issue?

Nope. I have no data to suggest it's worthwhile.

How do you envision passing this from the tool?

I haven't thought about it. Maybe we could make it so the library has a unique name so we can look directly for it?

ok. I'd suggest filing an issue, and including the general idea about how this should be solved.

flutter/flutter#99308

blasten · 2022-02-25T01:04:28Z

runtime/dart_isolate.cc

+  Dart_Handle libraries = Dart_GetLoadedLibraries();
+  intptr_t length = 0;
+  Dart_ListLength(libraries, &length);
+  for (intptr_t i = 0; i < length; ++i) {


this grows with the number of packages, right?

That's right. I suspect the order of magnitude will never make this an issue. It depends on Dart's implementation of their API but linearly searching through a couple thousand strings shouldn't be significant. Most of the string comparisons will fail on the first char check.

For an internal customer, this resulted in a >300ms regression on startup time on low end Android devices.

Isn't it more like ~275ms? I might be looking at the wrong thing.

Sorry, it was just under 300ms - a mean change of about 291ms on low end Android. This represented a 17% regression from before this patch. I think we should revert this.

blasten

LGTM

gaaclarke · 2022-02-28T19:04:14Z

@blasten can you give this another look. I decided to add an engine test. That will make the old code path easier to rip out once the engine PR lands.

blasten · 2022-02-28T19:51:56Z

runtime/fixtures/dart_tool/flutter_build/dart_plugin_registrant.dart

+bool didCallRegistrantBeforeEntrypoint = false;
+
+// Test the Dart plugin registrant.
+@pragma('vm:entry-point')


is there any difference between this file (or test) and

engine/runtime/fixtures/runtime_test.dart

Lines 164 to 185 in bfb5810

@pragma('vm:entry-point')

class _PluginRegistrant {

@pragma('vm:entry-point')

static void register() {

if (didCallRegistrantBeforeEntrypoint) {

throw '_registerPlugins is being called twice';

}

didCallRegistrantBeforeEntrypoint = true;

}

}

@pragma('vm:entry-point')

void mainForPluginRegistrantTest() {

if (didCallRegistrantBeforeEntrypoint) {

passMessage('_PluginRegistrant.register() was called');

} else {

passMessage('_PluginRegistrant.register() was not called');

}

}

?

Yep, the path of the file. The other one can be removed once we land the Framework PR.

longer is compiled in the root library.

…utter/engine#31418)

* e97c801 Started looking for the dart plugin registrant at runtime (flutter/engine#31418) * 9ebbc6e Standardize the embedder file name. (flutter/engine#31723) * e983ac8 Roll Skia from 1fc0c1c0f8af to 5deac304821b (2 revisions) (flutter/engine#31725) * afd334d added dart plugin registrant test executables to run_tests.py (flutter/engine#31726) * 0ff508b Standardize embedder archive name. (flutter/engine#31693) * 9c1daae Roll Fuchsia Mac SDK from qGsRXn7-D... to RIOZP5nXz... (flutter/engine#31730) * ceec638 Roll Fuchsia Linux SDK from _mwyhkd9H... to 7rB0Yrprs... (flutter/engine#31731) * 54050b4 Roll Skia from 5deac304821b to c3e7a2a00b0a (3 revisions) (flutter/engine#31732) * a5718ab Roll Dart SDK from c3e3126a598b to 34a5563af80d (6 revisions) (flutter/engine#31734)

…utter#31418)" This reverts commit e97c801.

gaaclarke · 2022-03-21T18:54:28Z

@alexmarkov we had to revert this because it caused a performance regression (300ms). This PR is related to the --source flag. Remember how it creates a library with a file uri? That's why I have to hunt for it.

You said in dart-lang/sdk#48144 (comment) that we could potentially make this generated code a package instead. If we got a package uri instead of a file uri we could look for it directly. Would that be your recommendation here?

It also seems unfortunate that this code take 300ms. It is essentially performing a couple thousand string comparisons. Maybe there is something we could do to speed that up.

If we go down the package route, how does one edit the .dart_tool/package_config.json? Is there an API for that or are you suggesting we open up that file and edit it directly?

alexmarkov · 2022-03-21T19:35:30Z

@gaaclarke I'm not sure what is the best way to deal with generated sources - so consider talking to someone more familiar with source code generation. FWIW here are a few options I can suggest:

We can probably save the file URI of the generated file and pass it to the engine.
We can generate a file alongside the main.dart in lib directory and call it main.g.dart. In such case its URI can be constructed from the main library URI (e.g. if main is package:foo/main.dart, then the generated file will have package:foo/main.g.dart URI).
As suggested before, we can generate a separate package for the generated file. Although package_config.json is just a JSON file, I would recommend to take a look at https://pub.dev/packages/package_config to work with those files as their format can change over time.

…1418)" (#32162) This reverts commit e97c801.

gaaclarke · 2022-03-21T23:00:27Z

@alexmarkov for option 1, could I compile the program with -Dflutter.dart_plugin_registrant=<file url> then use Dart_Invoke(string, "fromEnvironment", 1, "flutter.dart_plugin_registrant") from C++ to recall it?

It appears that's how FLUTTER_WEB_AUTO_DETECT works. It is a bit odd since I typically think of environment variables as being things defined at runtime, not compilation time.

alexmarkov · 2022-03-22T14:50:44Z

@gaaclarke Not sure you can use Dart environment defines for that. They are specified at compile time and used to evaluate const String/int/bool.fromEnvironment(...) constructors at compile time. These constructors are not guaranteed to work when called at run time (https://github.com/dart-lang/sdk/blob/69d66d9bd5a4ba79bc9040a60ec9e93df0a25255/sdk/lib/core/string.dart#L167-L170). At run time VM forwards querying of the environment to the embedder (Flutter engine in case of Flutter), and Flutter engine doesn't seem to call Dart_SetEnvironmentCallback in order to implement that. So it's likely that String.fromEnvironment won't work if called at run time from Flutter app.

…time (flutter#31418)" (flutter#32162)" This reverts commit ab6b671.

…1418)" (#32162) (#32267) This reverts commit e97c801. Co-authored-by: gaaclarke <[email protected]>

gaaclarke mentioned this pull request Feb 14, 2022

Starts using the --source flag to compile the dart registrant. flutter/flutter#98046

Merged

8 tasks

gaaclarke marked this pull request as ready for review February 14, 2022 18:54

gaaclarke requested a review from blasten February 14, 2022 18:54

flutter-dashboard bot added the needs tests label Feb 14, 2022

blasten reviewed Feb 25, 2022

View reviewed changes

blasten approved these changes Feb 25, 2022

View reviewed changes

gaaclarke force-pushed the find-plugin-registrant branch from b9c41d2 to 3a28e84 Compare February 28, 2022 19:00

gaaclarke force-pushed the find-plugin-registrant branch from 3a28e84 to 6b5c7fe Compare February 28, 2022 19:04

blasten reviewed Feb 28, 2022

View reviewed changes

blasten approved these changes Feb 28, 2022

View reviewed changes

gaaclarke added 4 commits February 28, 2022 13:40

Started looking for the dart plugin registrant at runtime since it no

9d281b0

longer is compiled in the root library.

renamed to dart_plugin_registrant

81be313

added engine unit test

13e9028

updated licenses

87f0eaa

gaaclarke force-pushed the find-plugin-registrant branch from 21f8dbc to 87f0eaa Compare February 28, 2022 21:47

gaaclarke merged commit e97c801 into flutter:main Feb 28, 2022

This was referenced Mar 1, 2022

Remove linear search for the dart plugin registrant flutter/flutter#99308

Closed

Added dart plugin registrant test executables to run_tests.py #31726

Merged

engine-flutter-autoroll mentioned this pull request Mar 1, 2022

Roll Engine from dc1dba505c33 to a5718ab3c33b (9 revisions) flutter/flutter#99346

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 1, 2022

e97c801 Started looking for the dart plugin registrant at runtime (fl…

d22590f

…utter/engine#31418)

gaaclarke added a commit to gaaclarke/engine that referenced this pull request Mar 21, 2022

Revert "Started looking for the dart plugin registrant at runtime (fl…

57feb58

…utter#31418)" This reverts commit e97c801.

gaaclarke added a commit that referenced this pull request Mar 21, 2022

Revert "Started looking for the dart plugin registrant at runtime (#3…

ab6b671

…1418)" (#32162) This reverts commit e97c801.

gaaclarke added a commit to gaaclarke/engine that referenced this pull request Mar 22, 2022

Revert "Revert "Started looking for the dart plugin registrant at run…

42a834a

…time (flutter#31418)" (flutter#32162)" This reverts commit ab6b671.

gaaclarke mentioned this pull request Mar 23, 2022

Dart Plugin Registrant - Reland e97c8016e398 #32189

Merged

8 tasks

godofredoc added a commit that referenced this pull request Mar 25, 2022

Revert "Started looking for the dart plugin registrant at runtime (#3…

6b4b96b

…1418)" (#32162) (#32267) This reverts commit e97c801. Co-authored-by: gaaclarke <[email protected]>

	@pragma('vm:entry-point')
	class _PluginRegistrant {

	@pragma('vm:entry-point')
	static void register() {
	if (didCallRegistrantBeforeEntrypoint) {
	throw '_registerPlugins is being called twice';
	}
	didCallRegistrantBeforeEntrypoint = true;
	}

	}


	@pragma('vm:entry-point')
	void mainForPluginRegistrantTest() {
	if (didCallRegistrantBeforeEntrypoint) {
	passMessage('_PluginRegistrant.register() was called');
	} else {
	passMessage('_PluginRegistrant.register() was not called');
	}
	}

Conversation

gaaclarke commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Feb 14, 2022

Uh oh!

gaaclarke commented Feb 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Feb 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Mar 21, 2022

Uh oh!

alexmarkov commented Mar 21, 2022

Uh oh!

gaaclarke commented Mar 21, 2022

Uh oh!

alexmarkov commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gaaclarke commented Feb 12, 2022 •

edited

Loading

alexmarkov commented Mar 22, 2022 •

edited

Loading