Skip to content

Add asset manifest parsing benchmark#112836

Merged
auto-submit[bot] merged 11 commits intoflutter:masterfrom
andrewkolos:add-asset-manifest-parsing-benchmark
Oct 7, 2022
Merged

Add asset manifest parsing benchmark#112836
auto-submit[bot] merged 11 commits intoflutter:masterfrom
andrewkolos:add-asset-manifest-parsing-benchmark

Conversation

@andrewkolos
Copy link
Contributor

@andrewkolos andrewkolos commented Oct 4, 2022

This change adds a benchmark for loading and parsing the asset manifest from storage. It does this be mimicking the implementation of AssetImage.obtainKey, using AssetBundle.loadStructuredData to load and parse the file. It clears the bundle's cache between iterations.

Running this locally on an emulated Pixel 5 (API 33), I noticed some interesting results:

  • 1 iteration: ~50ms
  • 10 iterations: ~200ms
  • 100 iterations: ~950 ms
  • 1000 iterations: ~2900 ms

The runtime does not vary linearly with iteration count. Perhaps this is due to JIT optimization. Otherwise, I missed something.

TODO: I'm considering exposing the _manifestParser function as it's now duplicated in at least two tests.

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.

@andrewkolos andrewkolos added tool Affects the "flutter" command-line tool. See also t: labels. a: assets Packaging, accessing, or using assets labels Oct 4, 2022
@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Oct 4, 2022
@jonahwilliams
Copy link
Contributor

release mode is AOT, so its likely not due to JIT. Instead it could be a mix of memory locality, scheduling (due to the async nature of asset loading), and garbage collection. Its reasonable that the cost varies per count.

If you wanted to rule out cost of loading the asset from memory, you could instead cache the raw bytes and measure the cost of utf8 conversion followed by JSON parsing.

@jonahwilliams
Copy link
Contributor

Also, you won't really get very real results running this on an emulator. I would investigate what the performance is like on a real device too

@andrewkolos
Copy link
Contributor Author

andrewkolos commented Oct 4, 2022

release mode is AOT, so its likely not due to JIT. Instead it could be a mix of memory locality, scheduling (due to the async nature of asset loading), and garbage collection. Its reasonable that the cost varies per count.

Ooh I failed to consider memory locality. I would love to learn how scheduling and GC would affect this though 👀

If you wanted to rule out cost of loading the asset from memory, you could instead cache the raw bytes and measure the cost of utf8 conversion followed by JSON parsing.

This could work. However, one thing that irks me is that moves further away from testing the actual implementation/interface (AssetImage.obtainKey). Perhaps this is unavoidable in this situation without moving to some more complicated E2E test.

Also, you won't really get very real results running this on an emulator. I would investigate what the performance is like on a real device too

Gee, you sure weren't kidding!
image

(edit: typos)

@andrewkolos
Copy link
Contributor Author

If you wanted to rule out cost of loading the asset from memory, you could instead cache the raw bytes and measure the cost of utf8 conversion followed by JSON parsing.

I just made this change, and the more I think about it, the more it makes sense in that it most closely tests the decoding+parsing change we are looking to make. Still, one thing this neglects is that the binary encoding may result in faster load from storage since the file will be smaller. Do you think this might be worth putting back into the test loop? The thing I wouldn't like about this is, if we ever changed the iteration count in the future, it would invalidate* previous data due to storage locality--though I do wonder if changing the iteration count invalidates data regardless.

*By "invalidate", I mean that it would no longer be useful to compare metrics from before the change to after the change.

@jonahwilliams
Copy link
Contributor

I think it depends on how much variation you see when running on a real device. If the numbers are fairly stable around ~30 iterations, I think its fine if it doesn't scale completely linearly. We only need to worry about the delta between these numbers and the updated numbers

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Oct 5, 2022
@andrewkolos andrewkolos marked this pull request as ready for review October 5, 2022 19:53
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andrewkolos andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 7, 2022
@auto-submit auto-submit bot merged commit eec8d9d into flutter:master Oct 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Oct 7, 2022
@andrewkolos andrewkolos deleted the add-asset-manifest-parsing-benchmark branch April 21, 2023 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: assets Packaging, accessing, or using assets autosubmit Merge PR when tree becomes green via auto submit App c: contributor-productivity Team-specific productivity, code health, technical debt. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants