Make tracing safe #32042
Make tracing safe #32042
Conversation
|
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. |
|
I opted not for a revert because the original patch no longer reverts cleanly and I do not want to introduce new bugs while wading through the conflicting changes. |
|
And this patch does have a test, although it does not currently have a direct test of the new tracing utility class.... |
| @@ -33,6 +32,7 @@ | |||
| import io.flutter.embedding.engine.plugins.service.ServiceAware; | |||
| import io.flutter.embedding.engine.plugins.service.ServiceControlSurface; | |||
| import io.flutter.embedding.engine.plugins.service.ServicePluginBinding; | |||
| import io.flutter.util.TraceSection; | |||
There was a problem hiding this comment.
sorry, I'm confused. Where is this defined? Is this PR missing a commit?
There was a problem hiding this comment.
Whups! There were two missing files in what I pushed up, fixing that now.
|
I had some more missing stuff here.. it should build now. |
| * @param sectionName The string to display as the section name in the trace. | ||
| */ | ||
| public TraceSection(@NonNull String sectionName) { | ||
| sectionName = sectionName.length() < 127 ? sectionName : sectionName.substring(0, 127); |
There was a problem hiding this comment.
Should we try to add ... at the end of the section name to make it more obvious that the trace is truncated? Abruptly truncating it at 127 characters could look like a bug
| import androidx.annotation.NonNull; | ||
| import androidx.tracing.Trace; | ||
|
|
||
| public final class TraceSection implements AutoCloseable { |
There was a problem hiding this comment.
It looks like AutoCloseable is only supported on API >=19. Looking at https://docs.flutter.dev/development/tools/sdk/release-notes/supported-platforms, this seems ok but it might break "Best effort platforms tested by the community"
There was a problem hiding this comment.
right. I have an AI to ask customers if we can drop support. For the time being, I don't think we can break without ensuring this is ok.
cc @xster
There was a problem hiding this comment.
This is unfortunate. My understanding was that we don't have to continue to support work arounds for these API levels, but it's not a huge deal to continue to support this one. I'll avoid using AutoCloseable for now.
There was a problem hiding this comment.
Updated to make the TraceSection class just expose static methods to call in place of begin/endSection. Someday we can refactor it again to use try-with-resources to remove the boilerplate/avoid mistakes about when to call endSection.
dnfield
added
the
waiting for tree to go green
* 9f13454 [windows] Support win_debug_x86 compilation target in engine (flutter/engine#30417) * c5e958e Make tracing safe (flutter/engine#32042) * cf875d4 [macOS] Forward key events to NSTextInputContext when there's composing text (flutter/engine#32051) * ffd5c9c Roll Fuchsia Linux SDK from Ee9OX2o6P... to mVqiTwaVa... (flutter/engine#32055) * b0018c3 Roll Skia from a9bc6c643791 to 8afe53fd76d3 (1 revision) (flutter/engine#32056) * df12321 Roll Skia from 8afe53fd76d3 to 0d81bc7bb04e (2 revisions) (flutter/engine#32057) * 398a274 Roll Fuchsia Mac SDK from jvlI1s78T... to vWlaMIVkM... (flutter/engine#32060) * 3ffa9cf Roll Skia from 0d81bc7bb04e to e253cc3e55d3 (1 revision) (flutter/engine#32063) * 275cd2b [web] Position spans absolutely within paragraph (flutter/engine#31907) * 46c2cca Roll Skia from e253cc3e55d3 to 9301fe3779bb (1 revision) (flutter/engine#32064)
|
in which stable version this fix will be available? |
|
AFAIK this was cherry picked in to 2.12. |
Fixes flutter/flutter#99980
@chinmaygarde fyi
This will likely want a cherry pick.
Forbids importing
android.tracing.Traceorandroidx.tracing.Traceoutside of a new wrapper class that uses these as an AutoCloseable.We still do this in release mode because tracing is still meaningful in release mode.