WorkerTransfer benchmark 3x slower on main compared to v2.1.1

[wipfli]

The WorkerTransfer benchmarks is 3x slower on main compared to v2.1.1.

@HarelM @birkskyum, might recent changes have something to do with this?

Steps to reproduce:

git checkout main
npm ci
npm run benchmark -- WorkerTransfer

                          v2.1.1      main 0263cc0 
 WorkerTransfer      19.9715 ms⚠️       93.1357 ms      +73.1641 ms

[wipfli]

Yes, this is more like a factor of 4.5. But last time I run it, it was 30 ms. Would be helpful if someone could run this command and post the benchmark results here. Maybe it is machine-specific...

[birkskyum]

Hmm.. the commits that potentially could be involved off the top of my head would be #976, #974, #968. They were all merged recently.

We should probably figure this out and cut a new release soon, as a lot has happened since the last patch release.

[HarelM]

#941 also comes to mind as it adds another type to web worker transfer, which is exactly that test. ?

[HarelM]

I agree we should release a new version soon, I doubt that changing the worker code that was a simple replacement of file would've affected the performance, but everything is possible... :-)

[birkskyum]

http://localhost:9966/test/bench/versions#WorkerTransfer

[birkskyum]

or on cli npm run benchmark -- WorkerTransfer

[HarelM]

See my comment here:
#913 (comment)
I see this error in the console, I think it is related to this #913 PR...
Can anyone confirm this is not only happening to me?

[wipfli]

Happened to me too.

[birkskyum]

But that was merged before 2.1.1, which seems to perform okay, but it could still be origin of the warnings.

[HarelM]

It's a different issue, I don't think it is related to the worker transfer performance, or 2.1.1 is faster because this fails fast :-)

[HarelM]

This probably needs a bisect... :-( too late for me though...

[birkskyum]

I found something... checking out the commit from v 2.1.1, and somehow it's different from v2.1.1 column.

The third colum is commit (f47e53c), and it should really be the same as the first.

[HarelM]

If we uploaded a generated benchmark file that doesn't properly use the web worker might it explain it?
I think we should test against an older version maybe?

[HarelM]

I'm talking about this PR: #704

[HarelM]

I wanted to see if this happens also in previous version, and I'm getting the following results:
npm run benchmark -- WorkerTransfer --compare v1.15.2 --compare v2.1.1

                            1.x cb3a420                    v2.1.1  move-bench-dirty 99107f3 
 WorkerTransfer              16.4234 ms                95.4176 ms               101.4773 ms 

npm run benchmark -- WorkerTransfer --compare v2.1.0 --compare v2.1.1

                                 v2.1.0                    v2.1.1  move-bench-dirty 99107f3 
 WorkerTransfer              16.6740 ms                98.1929 ms               106.5554 ms

So it seems that for some reason the first run is very fast but the next runs are not. I don't have a clue what can cause this though.

[birkskyum]

My current hypothesis is that workers are created for the first run, but never terminated, and thus maybe blocking them from launching in the other runs. So if I'm not mistaken, this indicates that web workers make a huge difference, and we could in principle release 2.1.2 and expect it to have the same performance as 2.1.1.

[HarelM]

I think we should release 2.1.2, even today.
We need to iron out any issues that might have been introduced from the build changes before we release the 3D. The community will hopefully help in that aspect.

[birkskyum]

I have investigated a bit, and the only way the worker poll is being terminated is through:

Map._updateStyle() > Style._remove(); > Dispatcher.remove(); > WorkerPool.release()

So if we could also release the worker pool after each benchmark, that would probably help.

[birkskyum]

Also, I tried to add a worker.terminate() in the WorkerTransfer teardown function, as we should clean it up properly.

[birkskyum]

All the map workers start when we trigger

function loadScript(src) {
            return new Promise((resolve, reject) => {
                const s = document.createElement('script');
                s.src = src;
                s.onload = resolve;
                s.onerror = reject;
                document.head.appendChild(s);
            });
        }

It would be much easier if we could use the maplibre api directly from the benchmarks, instead of going through benchmarks_generated.js

[HarelM]

Yup, see #982 .
But it's not an easy task as the current implementation checks the performance deep into the library code which is very powerful.

[github-actions]

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 30 days.