Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Engine startup event timed after VM initializes #35713

Merged
yaakovschectman merged 5 commits into from
Aug 31, 2022
Merged

Engine startup event timed after VM initializes #35713

Changes from 1 commit
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
7 changes: 4 additions & 3 deletions runtime/dart_vm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,11 +470,12 @@ 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()) {

@chinmaygarde chinmaygarde Aug 26, 2022

Copy link
Copy Markdown
Contributor

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?

@yaakovschectman yaakovschectman Aug 26, 2022

Copy link
Copy Markdown
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?

@dnfield dnfield Aug 26, 2022

Copy link
Copy Markdown
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.

@yaakovschectman yaakovschectman Aug 26, 2022

Copy link
Copy Markdown
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?

@chinmaygarde chinmaygarde Aug 26, 2022

Copy link
Copy Markdown
Contributor

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.

@yaakovschectman yaakovschectman Aug 26, 2022

Copy link
Copy Markdown
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.

@cbracken cbracken Aug 27, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@yaakovschectman yaakovschectman Aug 30, 2022

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

int64_t micros = Dart_TimelineGetMicros();
Dart_TimelineEvent(
"FlutterEngineMainEnter", // label
settings_.engine_start_timestamp.count(), // timestamp0
Dart_TimelineGetMicros(), // timestamp1_or_async_id
Dart_Timeline_Event_Duration, // event type
micros, // timestamp0
Comment thread
yaakovschectman marked this conversation as resolved.
Outdated
micros, // timestamp1_or_async_id
Dart_Timeline_Event_Instant, // event type
0, // argument_count
nullptr, // argument_names
nullptr // argument_values
Expand Down