Handle deprecation of Dart_TimelineEvent Embedder API

[derekxu16]

This PR changes usages of Dart_TimelineEvent to Dart_RecordTimelineEvent as Dart_TimelineEvent was deprecated in https://dart-review.googlesource.com/c/sdk/+/308721.

[derekxu16]

This doesn't do anything because flow events don't bind to async events in Chrome's trace format.

[dnfield]

This needs a test, and it looks like CI isn't happy.

What's a case where we'd ever want more than 1 flow ID? It looks like it's always either 1 or 0. Can we just make the API take a single parameter and assume the count is always 1 if it's not nullptr?

[derekxu16]

This needs a test, and it looks like CI isn't happy.

I haven't landed the Dart CL yet, because it will break Dart SDK into Flutter Engine rolls. I'd like to get this PR approved first, and then I'll be able to include these changes with the rolling of the embedder API changes. CI will only pass when these changes are included with the rolling of the embedder API changes. I'm not sure how to write a test for this, do you have an example I can follow?

What's a case where we'd ever want more than 1 flow ID? It looks like it's always either 1 or 0. Can we just make the API take a single parameter and assume the count is always 1 if it's not nullptr?

Animator has events that are linked to multiple flow events

engine/shell/common/animator.cc

Line 71 in adca97c

while (!trace_flow_ids_.empty()) {

engine/shell/common/animator.cc

Line 264 in adca97c

while (!self->trace_flow_ids_.empty()) {

[dnfield]

I haven't landed the Dart CL yet, because it will break Dart SDK into Flutter Engine rolls

If we can in any way avoid the need for a manual roll we should. It makes things like reverts/relands much harder, and ends up making the release process more complicated. Can we avoid that here?

[dnfield]

(Thanks for the pointer about multiple flow ids)

[derekxu16]

If we can in any way avoid the need for a manual roll we should. It makes things like reverts/relands much harder, and ends up making the release process more complicated. Can we avoid that here?

I think a manual roll has to be done here, because on the Dart side we are adding new required parameters to the Dart_TimelineEvent function, which will make all existing calls to Dart_TimelineEvent have the wrong number of arguments. We can't get around it by adding them as optional parameters, because dart_tools_api.h is only supposed to use C features (https://dart-review.googlesource.com/c/sdk/+/302963/comment/a70bff54_1920bed7/).

We can make the number of Engine changes included in the manual roll smaller though. There aren't many direct calls to Dart_TimelineEvent, so we can just pass flow_id_count=0 and flow_ids=nullptr to those when doing the roll. Then we can update the TRACE_EVENT_* macros in a separate PR. Would that be any better?

[dnfield]

Why can't we deprecate the old method and create a new one?

[derekxu16]

I just discussed this with @bkonyi. We would prefer not to go through the extra deprecation / cleanup process since this function is only used in a couple of places in the Engine. Let us know if you still strongly desire to avoid the manual roll.

[zanderso]

@derekxu16 @bkonyi It looks like there's a straightforward way to do a soft transition for this API, so that's what we should do. Manual rolls are for emergencies or cases where a soft transition is impossible.

[flutter-dashboard]

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.

[derekxu16]

I'll be able to test this in https://github.com/flutter/engine/blob/main/testing/dart/observatory/tracing_test.dart once https://dart-review.googlesource.com/c/sdk/+/303022 lands. I need that CL to land because it will allow me to decode Perfetto-format trace protos and check that flow IDs are present in them. That CL is currently blocked for the reasons described here: https://dart-review.googlesource.com/c/sdk/+/303022/comment/d77751a4_f9e8fea8. I'll revist this PR once that CL lands.

[derekxu16]

The Linux linux_unopt check is failing because it's detecting a leak. I think it's because flutter_tester is not cleaning up the VM properly. I added print statements to dart::Cleanup in third_party/dart/runtime/vm/dart.cc, built the engine with --no-prebuilt-dart-sdk, ran the --type=dart tests, and never saw the string that I printed.

We have a test in the VM that passes flow ID arguments to Dart_RecordTimelineEvent, and ASAN doesn't detect leaks when running that test. https://github.com/dart-lang/sdk/blob/b64fc318e715081c3ce99ae3242e598886cb3ce2/runtime/vm/dart_api_impl_test.cc#L10416

[flutter-dashboard]

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

[dnfield]

(From offline discussion - we leak the VM by default, the tester should not need to leak the VM, setting it to not leak the VM makes ASAN happy)

[dnfield]

LGTM as long as bots are happy - I rekicked one that seemed like an infra failure.