Rename debugProfilePlatformChannels to a constant that works in release mode

[jiahaog]

When it comes to startup profiling, it is very helpful to look at platform channels. debugProfilePlatformChannels today only works in debug and profile mode. Unfortunately, using profile mode is less accurate for startup profiling, because of the service isolate introducing additional overhead.

This PR allows this toggle to work in release mode. Note that there are two parts to debugProfilePlatformChannels:

• Adding timeline events
• Logging statistics about platform channels

I also considered adding a separate toggle to limit the scope of this change to the former, but that seems like complexity that we might not need at this time.

Towards #102189

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, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

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

[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 or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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]

Code looks good, needs renaming though.

[gaaclarke]

You'll have to remove the debug prefix from the variable. Things with that prefix should be guaranteed to only run in debug builds.

[jiahaog]

My interpretation of the "debug" prefix was that it's more of a debugging toggle - I couldn't find any other variables in any of the debug.dart files in the framework that don't have the prefix. On the other hand, perhaps this is the first toggle that we want to be usable in release mode.

[gaaclarke]

It's my understanding that anything with the debug prefix should be tree-shaken in release builds. Maybe we'll have to move it out of the debug.dart file in that case.

[jiahaog]

Sure, I moved it to platform_channel.dart

[gaaclarke]

I'm a bit hesitant to accept this because this will have an effect on platform channel performance on release builds now. I'm not too excited about that, what do you think @dnfield ? I'd feel better if we ripped these out if we don't supply a flag. I know that that is a lot of work though since I've done that before.

[jiahaog]

Will it have any effect on performance when disabled? It is disabled by default so I'd only expect a small AOT size increase.

(The debugProfilePlatformChannels toggle was renamed to profilePlatformChannels and moved into platform_channel.dart, but it still remains disabled by default)

[gaaclarke]

Will it have any effect on performance when disabled? It is disabled by default so I'd only expect a small AOT size increase.

Yep, every platform channel message will now be doing a boolean check against wether it was turned on or not. That wasn't the case previously because the treeshaker would remove that kRelease check since kRelease is a constant. I think ideally we want a way to turn this on so it will get built into an app on release and ripped out on release without some flag.

[jiahaog]

In practice removing a boolean check is probably a micro-optimization and probably wouldn't have any effect on performance as a whole. I remember you mentioned that there were some benchmarks for platform channels though, can you point me to them to verify this?

We can also make profilePlatformChannels const, but that would mean that developers have to patch the framework instead of being able to turn it on within their own Dart code.

[gaaclarke]

In practice removing a boolean check is probably a micro-optimization and probably wouldn't have any effect on performance as a whole. I remember you mentioned that there were some benchmarks for platform channels though, can you point me to them to verify this?

We can also make profilePlatformChannels const, but that would mean that developers have to patch the framework instead of being able to turn it on within their own Dart code.

My take is that anything that happens at greater frequency than the frame rate we should do our best to make it as fast as possible. Boolean checks aren't as straightforward in dart as they are in other languages because of boxed types. There are these things called kernel transforms that allow people to change compiled code. Thats how we rip out asserts. I'd prefer profilePlatformChannels was a constant that got switched on with a kernel transform. No need to edit the framework code. Let me see if i can find where the kernel transforms are, they are in the dart repository.

[gaaclarke]

Here is some build info we could use to turn on features like this:

flutter/packages/flutter_tools/lib/src/build_info.dart

Line 21 in 52ef9d8

class BuildInfo {

Maybe there is already something like this you can already latch onto so you don't need to make a new option. Jacob would be a good person to ask.

[jiahaog]

I made the toggle a const instead. I think in the near term, we shouldn't worry too much about usability concerns and having to patch this file to turn it on is an acceptable compromise. In the longer term, this should be something integrated into devtools and we can probably defer a proper way to turn it on at runtime for then. What do you think?

[gaaclarke]

That sounds good to me. Thanks. Sorry, it isn't as straightforward to do this as I'd like.

[auto-submit]

auto label is removed for flutter/flutter/134922, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

• The status or check suite Linux analyze has failed. Please fix the issues identified (or deflake) before re-applying this label.

[dnfield]

Would it help to make this a const bool.fromEnvironment?

I can't remember if that works well in google's build environment or not. It also may have issues on web.

[jiahaog]

That could work and we could wire it up through the build rules to have something like:

flutter_application(
  profilePlatformChannels = True,
)

But I'm not sure if this is so helpful in the near term. Since we probably don't want users to turn that flag on permanently, having to patch their flutter_application is very similar to having to patch this file locally when profiling. For now I'm working on documentation to do things manually, but in the long term this can ideally be some toggle in devtools.

[auto-submit]

auto label is removed for flutter/flutter/134922, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

[jiahaog]

I overwrote google testing because it seems like there's some infrastructure issue. I also checked that there are no other users of debugProfilePlatformChannels in Google3.