base64 decoding is pretty slow. Have you benchmarked this versus the new binary for and/or the old JSON format?

Have you benchmarked this versus the new binary for and/or the old JSON format?

@jonahwilliams Nope, do those benchmarks exist or do they need to be written?

See https://github.com/flutter/flutter/blob/master/dev/benchmarks/microbenchmarks/lib/foundation/decode_and_parse_asset_manifest.dart

This change will also still require us to make an additional network requst before any assets can be downloaded.

@jonahwilliams @yjbanov

I took the microbenchmark that Jonah suggested here (thanks!), and modified it slightly to run on the web.

I had to:

• Add a web/index.html (I actually did flutter create --platform web .)
• Changed the number of iterations from 1000 to 10000 (with 1k, each time I refreshed the benchmark, master would be even faster, so I assumed this was the browser warming up. I raised the number of iterations to try to give a chance to all benchmarks to be "similarly warm" without having to run them several times).

I ran the benchmark in master and the 2 PRs. I ran each example like this:

refresh-tools && flutter clean && flutter run -d web-server --target lib/foundation/decode_and_parse_asset_manifest.dart --web-port 7357 --release

(refresh-tools is a small command that I have to delete the flutter_tools cache, to ensure that changes actually work)

Here's the results after 10k iterations:

master	#130419	#131382 (this)
57078ms	58278ms	59956ms

I only ran each microbenchmark once in my gLinux machine, so I take the numbers with a healthy amount of skepticism. Squinting a little bit, we could say that "this" is 2878ms slower than master, so I guess downloading a ~30% larger file and json-base64-decoding it incurs in an (average(, not the best for benchmarks)) ~0.3ms penalty.

It also seems that #130419 is still downloading the AssetManifest.bin on the web, which is hurting it badly in this benchmark, I expected it to wipe the floor with the other two branches (I don't know exactly if I messed up here, but it seems that the global window array is being set, so at least some of the changes are being applied. I can also take a look there and see what's up.)

FYI @andrewkolos maybe you can help out with the benchmark issue here? Do we need to use a different method to load and parse the manifest?

Thanks for benchmarking this, @ditman! Looks like there's no material performance difference between these approaches. If so, I'd say let's go with the simplest solution.

@yjbanov Andrew's PR should be much, much faster in this microbenchmark (because it should NOT hit the network at all), I'm shaving that yak now to find why is it downloading the manifest (when it's set in the DOM already)

I pushed a fix to @andrewkolos' PR, and NOW it's chugging!

11ms for 10k iterations of parsing the manifest (now that the bytes are getting inlined, there's not much work to do for the loader, other than juggling the bytes coming from the JS global).

/cc @jonahwilliams @yjbanov.

What are the numbers here? Sorry its a bit hard to follow the different issues/threads.

Heya, sorry for the silence--I was out at the tail end of last week.

I initially considered generating a separate version of the asset manifest for web, but I wanted to cut out the network request for the asset manifest to speed up (initial) asset loading and avoid having all pending asset loads depending on a single network request). However, if we are willing to delay addressing these properly (i.e. fixing #118115), I don't mind this adopting this solution. The other reason I avoided a solution like this one is that I wanted to avoid having to write/maintain a bunch of logic to construct/consume a JSON file, but your solution of just taking the existing bin data and encoding it as a string is pretty clever and doesn't strike me as too slow or data-consuming.

Barring strong opinions from others, I'd like to adopt this PR and close my original PR (after a few days or so).

Thanks @andrewkolos! I'll take a second look at the tests I broke here and get this one ready to land. I'd like to give this version a shot before we do drastic changes to the build process.

We'll eventually tackle those in #118115, so we'll be sure to include both the assets manifest alongside the fonts' one.

Agh, I was forgetting to pass --platform=chrome to my flutter test commands, that's why things were working locally but still failing in CI!

I've checked in flutter/packages because I remember pushing a fix wrt the AssetManifest a while ago, however all tests that may use that TestAssetBundle seem to be skipped on the web (code).

For people from the future tracking a flutter/packages failure on the web to this PR, you need to apply a fix similar to this: b3f91d9

@andrewkolos offered to prepare a g3Fix for this so the "Google Testing" check passes and the PR can land. (It may be this PR that one that lands, or some other that @andrewkolos owns so he can do any changes he needs!)

One challenge I failed to foresee with this is the amount of change that might be required in g3. Unlike past asset manifest changes, this adds a file that is conditionally included in the build. This makes updating tests and build rules more difficult. In addition (though not as important), these kind of changes tend to break tests for customers that mock/fake AssetBundle in their tests. One of the reasons I initially liked this PR over my initial PR was it's relative simplicity. However, when considering g3 and testing, it may not be so simple.

It's possible that updating g3 to work with this PR might not be that complicated, but there are unknowns. Given that I've already invested at least 3x as much time as I wanted to on this issue, I'd like to go with the option that will probably have the fewest surprises. As a result, I may revisit my previous PR (#130419).

It's possible that updating g3 to work with this PR might not be that complicated, but there are unknowns.

OK, I guess I'll give that a shot too.

@andrewkolos is right, this is a world of hurt internally, what a shame :/

It's possible that updating g3 to work with this PR might not be that complicated, but there are unknowns.

OK, I guess I'll give that a shot too.

@andrewkolos is right, this is a world of hurt internally, what a shame :/

Do you have opinions against the core idea of #130419? As far as I can tell, @jonahwilliams and @yjbanov are satisfied with the core idea, as long as it uses a public dart API (as opposed to a hidden JS global). I really don't want to foist a bunch of g3 work on you. I'm happy to rework #130419 instead (though I can only hope g3 doesn't use custom code for generating the web entry point 🙃--I never got to find out because FroB bugged out and refused to test the PR).

I have a g3fix ready for this, applying.

I can only hope g3 doesn't use custom code for generating the web entry point 🙃

@andrewkolos sorry, I've just seen this message 😅

In fact, g3 does uses custom code to generate the web entrypoint (see cs//depot/google3/mobile/flutter/build_defs/internal/web_rules.bzl;l=195), so you'd still have to go through the g3fix process to land the other version of the change.

I prefer this PR because it keeps about the same complexity that we currently have in master, fixes the problem that some people have, and gives us some extra time to actually design what's going to be our web inlining strategy. (As Jonah said, it's not the responsibility of your PR to find that solution.)

I guess I'm trying to avoid having one-offs for each file we may want to inline, and find a consolidated solution. What if tomorrow we want to make the FontManifest.json a binary file? (I'd rather it became a .bin.json for initial web solution, than creating another different setter to the engine, etc...)

Hi @ditman

Do you know when this fix will land on a stable release?
We currently have the problem that in some companies the firewalls block the bin-file of our flutter-web-app.

Regards,
Florian

@fhuonder this landed in the 3.16.0 stable release from Nov. 15th, 2023.

(You can figure out in which channel this is available by looking at the "tags" section under the commit description in master, in this case: c7c9d8e it lists 3.16.0 (but not 3.13.0, which was the previous stable))

Thank you @ditman 🙏🏼