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

Conversation

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Aug 3, 2017

Part of the fix for flutter/flutter#11479.

@Hixie
Copy link
Contributor

Hixie commented Aug 3, 2017

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.

@yjbanov
Copy link
Contributor Author

yjbanov commented Aug 3, 2017

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 flutter_tools so it's opt-in for now. Would that work?

@Hixie
Copy link
Contributor

Hixie commented Aug 3, 2017

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.

@yjbanov
Copy link
Contributor Author

yjbanov commented Aug 3, 2017

Ran microbenchmarks, memory and size benchmarks: results

@Hixie
Copy link
Contributor

Hixie commented Aug 3, 2017

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?

@yjbanov
Copy link
Contributor Author

yjbanov commented Aug 3, 2017

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.

@yjbanov
Copy link
Contributor Author

yjbanov commented Aug 3, 2017

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.

@yjbanov
Copy link
Contributor Author

yjbanov commented Aug 4, 2017

What say ye?

@Hixie
Copy link
Contributor

Hixie commented Aug 4, 2017

If the benchmarks show no cost, then let's do it!

@yjbanov
Copy link
Contributor Author

yjbanov commented Aug 4, 2017

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.

@yjbanov yjbanov merged commit ff50334 into flutter:master Aug 4, 2017
@yjbanov yjbanov deleted the async-stacks branch June 22, 2021 21:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants