Add support for external workers in control CSP version

[pramilk]

Changes

1. Add support for users to provide workers directly in CSP version
   • This will allow worker(maplibre-gl-csp-worker.js) and main (maplibre-gl-csp.js) JS to be downloaded in parallel. Currently users can provide maplibre.workerUrl which means that first maplibre-gl-csp.js has to download before maplibre-gl-csp-worker.js download can even begin. This helps improve CSP version quite a bit in initial load performance.
2. Added unit tests for this new functionality.
3. Added bunch of missing unit tests for some of the existing functionality in Web_Worker, global_worker_pool and worker_pool.

Launch Checklist

• Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
• Briefly describe the changes in this PR.
• Link to related issues.
• Include before/after visuals or gifs if this PR includes visual changes.
• Write tests for all new functionality.
• Document any changes to public APIs.
• Post benchmark scores.
• Add an entry to CHANGELOG.md under the ## main section.

[HarelM]

Why undefined as...?

[pramilk]

Not sure why I added it. Updated code.

[rotu]

I'm a little skeptical of this PR. Given this motivation, the natural solution seems to be to preload (<link type="preload"> / <link type="modulepreload">) or to have MapLibre do an equivalent rather than push the onus onto user code.

This will allow worker(maplibre-gl-csp-worker.js) and main (maplibre-gl-csp.js) JS to be downloaded in parallel. Currently users can provide maplibre.workerUrl which means that first maplibre-gl-csp.js has to download before maplibre-gl-csp-worker.js download can even begin. This helps improve CSP version quite a bit in initial load performance.

[HarelM]

Csp by definition is pushing things to the user.
I personally don't use it because of this complexity, so generally speaking, if there's a place to make things more complicated is with csp related stuff.
Having said that, if there's a simpler solution I would agree complicating things is less ideal...

[rotu]

Csp by definition is pushing things to the user.

I disagree. The only thing required to be CSP-compliant is to have the worker script served from the same server as the UI entrypoint (rather than a data or blob url). So for instance, populate the worker url as require.resolve('maplibre-gl-js/dist/maplibre-gl-csp.js') or import.meta.resolve('maplibre-gl-js/dist/maplibre-gl-csp.js').

[pramilk]

@rotu, We have tried preloading and it did not worked for Workers. Here is a sample: https://perfmeasure.blob.core.windows.net/$web/maplibre_CSP_preload.html

Here is the warning I see in console and also can see worker getting downloaded twice.

I have tried setting as to "worker", but it's not supported for preload.

Apart from this downloading issue, another reason to push creation of Worker ASAP is the time it takes the Worker to spin up after it's been initialized (Js compilation time and some other mysterious time). Regarding 'mysterious time', even if I initialize a worker and immediately post a message to it. Message is not received by the worker immediately after Js compilation is complete. So this change makes sure that by the time style is broadcasted to worker, its ready to receive it.

This Change along with PR(#2131), allows styles to be processed in parallel majority of the time. We have tried with prewarm and were able to get styles processed in parallel only a small % of time.

[rotu]

@rotu, We have tried preloading and it did not worked for Workers. Here is a sample: https://perfmeasure.blob.core.windows.net/$web/maplibre_CSP_preload.html

Here is the warning I see in console and also can see worker getting downloaded twice.

I have tried setting as to "worker", but it's not supported for preload.

Yeah it seems browsers don't implement preload for script workers :-(. Does prefetch work instead? Does modulepreload work?

Apart from this downloading issue, another reason to push creation of Worker ASAP is the time it takes the Worker to spin up after it's been initialized (Js compilation time and some other mysterious time). Regarding 'mysterious time', even if I initialize a worker and immediately post a message to it. Message is not received by the worker immediately after Js compilation is complete. So this change makes sure that by the time style is broadcasted to worker, its ready to receive it.

Absolutely. As soon as we know we need workers, we should be spinning them up - I don't think that means we should be creating workers outside of the library.

This Change along with PR(#2131), allows styles to be processed in parallel majority of the time. We have tried with prewarm and were able to get styles processed in parallel only a small % of time.

I'm not clear what you mean - is prewarm working or not? I think a prewarm function is a better interface than injecting workers (though it may have to be split off from the monolithic umd build in order to actually work).

[pramilk]

@rotu, We have tried preloading and it did not worked for Workers. Here is a sample: https://perfmeasure.blob.core.windows.net/$web/maplibre_CSP_preload.html
Here is the warning I see in console and also can see worker getting downloaded twice.
I have tried setting as to "worker", but it's not supported for preload.

Yeah it seems browsers don't implement preload for script workers :-(. Does prefetch work instead? Does modulepreload work?

Apart from this downloading issue, another reason to push creation of Worker ASAP is the time it takes the Worker to spin up after it's been initialized (Js compilation time and some other mysterious time). Regarding 'mysterious time', even if I initialize a worker and immediately post a message to it. Message is not received by the worker immediately after Js compilation is complete. So this change makes sure that by the time style is broadcasted to worker, its ready to receive it.

Absolutely. As soon as we know we need workers, we should be spinning them up - I don't think that means we should be creating workers outside of the library.

This Change along with PR(#2131), allows styles to be processed in parallel majority of the time. We have tried with prewarm and were able to get styles processed in parallel only a small % of time.

I'm not clear what you mean - is prewarm working or not? I think a prewarm function is a better interface than injecting workers (though it may have to be split off from the monolithic umd build in order to actually work).

#1 Prefetch also does not work. Just tried modulepreload, I don't see console warning with it, but network does show worker getting downloaded twice.

#2. Spinning the worker outside the library gives us the best shot at having the worker ready ASAP, as it does not have to wait for Library. This is showing faster overall load performance on Bing maps, even faster than non-CSP version with prewarm.

#3. Yes prewarm is working, buts it's just too late for the worker to be ready by the time main thread broadcast styles to it. Specially when style.json is cached. This is causing style to be processed on worker thread way past it's done on main thread. With PR(#2131), it will be even more suboptimal (time difference style broadcast time from main and received on worker).

[rotu]

#1 Prefetch also does not work. Just tried modulepreload, I don't see console warning with it, but network does show worker getting downloaded twice.

If it’s getting downloaded twice, it sounds like you may have disabled your browser cache.

#2. Spinning the worker outside the library gives us the best shot at having the worker ready ASAP, as it does not have to wait for Library. This is showing faster overall load performance on Bing maps, even faster than non-CSP version with prewarm.

So what is prewarm doing if it’s not spinning up workers?

#3. Yes prewarm is working, buts it's just too late for the worker to be ready by the time main thread broadcast styles to it. Specially when style.json is cached. This is causing style to be processed on worker thread way past it's done on main thread. With PR(#2131), it will be even more suboptimal (time difference style broadcast time from main and received on worker).

What is the point of prewarming if it the workers aren’t ready by the time we need them?

I’m thinking that our implementation of prewarm is broken

[rotu]

In looking into this more, our Worker implementation is very unsafe; the WorkerInterface type hardly specifies at all what the implementation needs to do.

Definitely not the kind of thing we should be exposing in the public interface.

[pramilk]

I relooked into it and modulepreload did worked (verified on webpagetest.org). However current support for it is limited to mostly Chromium browsers. Our most users (80+% users) are on these browsers.

I did some synthetic testing on webpageTest comparing external workers vs prewarm and I am not seeing much difference ( with and without this PR) when using modulepreload on a limited test size. Thanks for pointer to it.

I will run a A/B test on Bing maps and will report back in about 2 weeks, to see if we still see any improvement with external worker (after applying modulepreload). If they are at par, then there will be no need for this new functionality.

Thanks again for your time. Will report back in 2 weeks.

Here is the samples pages I tested
CSP with external Worker: https://perfmeasure.blob.core.windows.net/maplibre/maplibre_CSP_ExternalWorker.html
CSP with prewarm: https://perfmeasure.blob.core.windows.net/maplibre/maplibre_CSP_Prewarm.html
Regular: https://perfmeasure.blob.core.windows.net/maplibre/maplibre_mlStyle.html

[pramilk]

Update. We ran a A/B test with and without this change. There was not a statistically significant gain seen in the metrics that we log. Abandoning this PR for now. Thanks for all your comments.