-
Notifications
You must be signed in to change notification settings - Fork 6k
enable async stack traces in all modes #3948
Conversation
|
I believe we tried this once and it had significant undesireable performance implications. I recommend rolling it into the framework independent of other changes so that we can accurately assess the cost. |
|
Do you recall what you used as a benchmark? I ran our microbenchmarks (the only ones we can run in release mode today) and there was no change. I also eyeballed the performance overlay of Flutter Gallery and did not notice a slow-down. I can pipe this option through to |
|
I dunno. Maybe it was binary size? Or memory size? Options are usually a terrible idea. We should verify what the performance implications are, and if they're acceptable, we should just turn it on by default. If they're not acceptable, we should at a minimum document them in the docs for the option. |
|
Ran microbenchmarks, memory and size benchmarks: results |
|
Those results are unlikely (improvements when adding more work?). Can you run them a few times so we can get an idea of the error bars? |
|
I tested against tip of flutter/flutter, which is 10 engine commits behind my engine build. I'll sync and try again, and also will average the numbers to reduce noise, but the differences in the results are within our normal microbenchmark noise. |
|
Reran benchmarks again on engine rev 1de56a3 with and without causal_async_stacks and the delta is still within the normal noise, and the absolute numbers are not very different from the first run on the benchmarks. |
|
What say ye? |
|
If the benchmarks show no cost, then let's do it! |
|
The cost might be hidden in the noise. My plan is watch the benchmarks and revert if I see regressions. It's a one-line change; very easy to revert. |
Part of the fix for flutter/flutter#11479.