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.

Sign up for GitHub

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

Engine startup event timed after VM initializes #35713

Merged
merged 5 commits into from
Aug 31, 2022
Merged

Engine startup event timed after VM initializes #35713

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac334e5
Engine startup event timed after VM initializes
yaakovschectman Aug 23, 2022
e2d0691
Formatting
yaakovschectman Aug 25, 2022
a4f0dc8
Update FlutterEngineMainEnter event comment
yaakovschectman Aug 26, 2022
a11e0a4
Remove engine_start_time
yaakovschectman Aug 30, 2022
f53100c
Remove unused variable
yaakovschectman Aug 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions runtime/dart_vm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,14 +470,14 @@ DartVM::DartVM(std::shared_ptr<const DartVMData> vm_data,
// Use a duration event so about:tracing will consider this event when
// deciding the earliest event to use as time 0.
if (settings_.engine_start_timestamp.count()) {
Copy link
Member

Choose a reason for hiding this comment

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

Are there any users of engine_start_timestamp? If not, can we get rid of them. Why can't the caller specify the startup time instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can find, this was the only user. I am unsure what you mean by that. Caller to where?

Copy link
Contributor

Choose a reason for hiding this comment

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

Internal codesearch suggests this is the only usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make sure we are clear, is your suggestion to remove the engine_start_timestamp now and continue with the effort of this PR?

Copy link
Member

Choose a reason for hiding this comment

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

If no one is using the field in the settings struct, I suggest removing it altogether. If someone is using it, I suggest setting the field in the embedder you are working on. As it stands, this field will go unused and existing users (if any) will break.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not being used otherwise but our Flutter Engine, but should we make any consideration for 3rd party forks or embedders that we do not know about but might use it? settings_ is a private field so I would not expect there to be.

Copy link
Member

@cbracken cbracken Aug 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We promise stability to third-party embedders via the embedder API. Since Settings is non-public API, we'd simply need to make sure none of our own embedders that bypass the embedder API (e.g. iOS/Android/Fuchsia and internal embedders) are broken. If no one's using it (which seems to be the case) then you can safely delete it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Android shell assigns to this value but never uses it. Its only use is in generating this event

Dart_TimelineEvent(
"FlutterEngineMainEnter", // label
settings_.engine_start_timestamp.count(), // timestamp0
Dart_TimelineGetMicros(), // timestamp1_or_async_id
Dart_Timeline_Event_Duration, // event type
0, // argument_count
nullptr, // argument_names
nullptr // argument_values
int64_t micros = Dart_TimelineGetMicros();
Dart_TimelineEvent("FlutterEngineMainEnter", // label
micros, // timestamp0
Copy link
Member

Choose a reason for hiding this comment

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

How does this affect the benchmark value reported for non-Windows?

Does the comment above need to be adjusted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will shift the earliest timestamp forward by the time taken to initialize the VM. This seems a preferable outcome to having inconsistent and sometimes meaningless timestamps. The comment should be changed as it is not using a duration any more.

Copy link
Member

Choose a reason for hiding this comment

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

@dnfield Will this affect the startup metrics used by customer: money?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so but @jiahaog would know for sure

Copy link
Member

Choose a reason for hiding this comment

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

This will affect 1P startup metrics that read from the observatory (in profile mode), such as from Flutter driver. We read only the ts value of this metric, which is the timestamp0 parameter.

I don't think this is a blocker though, as customers can rebaseline their tests. We have also been directing customers to move away from such metrics to more general Android ones, and customer: money specifically does not use this metric.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you then see a need for further changes to this PR?

Copy link
Member

Choose a reason for hiding this comment

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

No, this should be fine. Thanks :)

micros, // timestamp1_or_async_id
Dart_Timeline_Event_Instant, // event type
0, // argument_count
nullptr, // argument_names
nullptr // argument_values
);
}
}
Expand Down