search

bikehopper / bikehopper-ui

Friendly bike+transit directions (frontend)
Other
26 stars 1 forks source link

Code split gpx2fit #369

Closed graue closed 3 months ago

graue commented 3 months ago

gpx2fit is a relatively large dependency (~300kB uncompressed) that many BH sessions will not use. This uses a dynamic import to defer loading it until a FIT file is downloaded.

To show it's doing something, the FIT button grays while the code is being downloaded, and if it fails to load, an alert is displayed. I tested this by adding a delay and by adding a throw, respectively.

graue commented 3 months ago

Note that other than the downloadFit callback this is mostly unchanged from the existing code. I just converted it to TypeScript.

Andykmcc commented 3 months ago

thanks for doing this.

i merged the other code splitting PR in hopes that it makes this easier. i'm not sure if it is best to delete this line given your work here or leave it as it. Ideally the big libraries that do not change often are in their own files so they can be cached independently from our application code. i'm not sure how that manual chunking interacts with dynamic imports.

maybe see what your GET reqs look like as is, then rebase main into here and see if the gpx lib is bundled into our own code or still left on it own (i expect and hope the gpx lib to be its own file).

graue commented 3 months ago

You are right. After rebasing on your change, we get a tiny bundle with only getFitBlob and not gpx2fit:

image

Will push a fix.

graue commented 3 months ago

Fixed:

image

Andykmcc commented 3 months ago

I think you are missing the point here.

You are right. After rebasing on your change, we get a tiny bundle with only getFitBlob and not gpx2fit

This is exactly how it should work and is desirable. The goal is to minimize the amount of data people need to load across the network. Breaking large libraries into their own file is good because they change far less often than our own app code. By making maplibre, gpx2fit and nivo their own files that only contain library code uses are more likely to have them cached from a previous load of the app. Even across changes/deploys for bikehopper-ui. If we leave these large libs in their own files and a user opens our app after a deploy the client will not redownload the same unchanged library. it will be cache locally, or at least in the cdn. This is very good and desirable. The files are all downloaded in parallel, the target size for each file is roughly 500kb to maximize download. Since everything to our site goes over http/2 with with connection reuse the clients doesn't have to do DNS and renegotiate TLS for each file. (see screen shots below). That said, there is a limit to how many files can be downloaded in parallel and we need to accounts for parsing and what is needed when. With all that in mind i firmly believe the ideal state for loading our app is the following bundles loaded the following ways:

Bundles:

First connection that is slow because TLS

Screenshot 2024-06-05 at 9 47 58 PM

All other calls to bikehopper.org, fast because of conn. resuse.

Screenshot 2024-06-05 at 9 48 03 PM
graue commented 3 months ago

I think it's silly and not beneficial for the GPX download to include a second separate file that is only 2.46 kB.

Similarly, since all the Turf modules we use are together only 13 kB, it's not worth the dev overhead to have to modify that configuration if we add or remove a Turf module.

I get what you're saying and agree on principle but splitting very small modules seems to be taking it to an absurd level. As you said, "the target size for each file is roughly 500kb to maximize download... there is a limit to how many files can be downloaded in parallel and we need to accounts for parsing and what is needed when." So 2 kB and 13 kB splits don't make a lot of sense to me.

graue commented 3 months ago

How about this? Splitting out React and related libs and React-Map-GL makes the index.js file 100 kB smaller than before. This doesn't create any extra tiny files, or use a long list of related modules that's likely to change.

vite v5.2.9 building for production...
✓ 1378 modules transformed.
build/index.html                           1.32 kB │ gzip:   0.62 kB
build/assets/index-wQ5T2K-i.css          122.95 kB │ gzip:  17.75 kB
build/assets/es-C5gUcrVZ.js                0.84 kB │ gzip:   0.20 kB
build/assets/es-HN-D9TZLjyG.js             0.84 kB │ gzip:   0.21 kB
build/assets/es-PH-DR8OZkjM.js             0.84 kB │ gzip:   0.21 kB
build/assets/es-GT-FuZfVQZp.js             0.84 kB │ gzip:   0.21 kB
build/assets/es-MX-DYm7RhX8.js             0.84 kB │ gzip:   0.21 kB
build/assets/es-CU-hNl4zoKN.js             0.84 kB │ gzip:   0.21 kB
build/assets/es-US-aSTXYusr.js             0.84 kB │ gzip:   0.21 kB
build/assets/en-Cn0RhvG2.js                0.85 kB │ gzip:   0.22 kB
build/assets/en-GB-DBxnGoEi.js             0.85 kB │ gzip:   0.22 kB
build/assets/en-CA-Bio751Cj.js             0.85 kB │ gzip:   0.22 kB
build/assets/en-IN-DPX6q_6Z.js             0.85 kB │ gzip:   0.22 kB
build/assets/polyfill-force-BzMOGqrQ.js    5.01 kB │ gzip:   1.85 kB
build/assets/web-vitals-BhWu73fZ.js        7.08 kB │ gzip:   2.60 kB
build/assets/en-CghXMlEL.js                9.90 kB │ gzip:   2.94 kB
build/assets/es-CVZwDaYl.js               11.62 kB │ gzip:   3.40 kB
build/assets/react-BKq7yv0m.js           138.43 kB │ gzip:  40.79 kB
build/assets/react-map-gl-CSlfFvEA.js    162.68 kB │ gzip:  52.87 kB
build/assets/@nivo-m4HmNUb3.js           254.46 kB │ gzip:  87.79 kB
build/assets/getFitBlob-CSFE_SvZ.js      310.47 kB │ gzip: 107.29 kB
build/assets/index-CynwAzCd.js           374.97 kB │ gzip: 117.04 kB
build/assets/maplibre-gl-D5NUTqdg.js     771.61 kB │ gzip: 201.58 kB
Andykmcc commented 3 months ago

The turf stuff can be bundles normally, whatever.

Nivo is large and not used till later so ideally its bytes aren't head of line blocking for bytes needed for the first contentful paint.

I think it's silly and not beneficial for the GPX download to include a second separate file that is only 2.46 kB.

In a way i agree. i'm not why you decided to dynamically import getFitBlob.js as opposed to leaving those 2kb bundled with everything else and just dynamically import the large library, gpx2fit. If that doesn't work for some reason then just letting it load the two files is fine. there is no meaningful difference in load time when one needs to load both, but there is when one only needs to load one file, which is much more often. Im optimizing for the common case. If we want to care about perf the biggest win will always been having less code to ship, since we aren't really talking about that, then the next biggest win is high cache hit rate. the best way to do that is to not invalidate the cache. so lets put the big, static crap in its own file that will live unchanged for months or years at a time. This goes back to the old pattern of having two files, app.js and vendor.js.

Andykmcc commented 3 months ago

How about this? Splitting out React and related libs and React-Map-GL makes the index.js file 100 kB smaller than before. This doesn't create any extra tiny files, or use a long list of related modules that's likely to change.

🤷 sure whatever. I thought about doing this at first but decided 700kb was good enough since we need react and react-gl out of the gate i didn't want to waste time tweaking this very much. my two cents would be to bundle them together into one react-vendor.js bundle since now they are both small-ish files (sub 500kb)

Andykmcc commented 3 months ago

i still think we should put gpx4fit in its own file since i believe should pretty much be bundled into four files:

frequently-changed-needed-for-initial-load.js infrequently-changed-needed-for-initial-load.js frequently-changed-not-needed-for-initial-load.js infrequently-changed-not-needed-for-initial-load.js

Then if any of these files to get over 500kb you create something like,

frequently-changed-needed-for-initial-load-0.js frequently-changed-needed-for-initial-load-1.js