Add cache for coveringTiles

[wipfli]

This is a follow-up on @JannikGM's pull request for caching covering tiles. I copy-pasted the code from his original pull request #105. Looks like benchmarks are not affected except for LayerBackground, which runs roughly 2x faster with the caching. Is this the behavior you expected, Jannik?

• [ ?] Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask! - I'm not sure, @JannikGM please comment about the origin of this code.
• [:whale: ] Briefly describe the changes in this PR. -> see Add cache for coveringTiles #105
• [ ⛷️ ] Post benchmark scores.
• [? ] Suggest a changelog category: bug/feature/docs/etc. or "skip changelog". @JannikGM, what would you say?

[wipfli]

If you want to run benchmarks against main locally, do this:

git clone ...
npm run ci
npm run start-bench

Open browser, go to http://localhost:9966/bench/versions?compare=main. If you just want to have a look at the LayerBackground benchmark, go to http://localhost:9966/bench/versions?compare=main#LayerBackground.

My benchmark results are: benchmarks-covering-tiles-wipfli.pdf

[github-actions]

Bundle size report:

Size Change: +68 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file	Before	After	Change
maplibre-gl.js	185 kB	185 kB	+68 B
maplibre-gl.css	9.49 kB	9.49 kB	0 B

ℹ️ View Details

Source file	Before	After	Change
rollup/build/tsc/src/geo/transform.js	3.76 kB	3.87 kB	+106 B

[HarelM]

Is this the only place the cache needs to be invalidated? Is this the right place? @JannikGM

[wipfli]

Shouldn't resize() also invalidate the cache?

[wipfli]

            transform.zoom = 0;
            expect(transform.coveringTiles(options)).toEqual([]);

            transform.zoom = 1;
            expect(transform.coveringTiles(options)).toEqual([
                new OverscaledTileID(1, 0, 1, 0, 0),
                new OverscaledTileID(1, 0, 1, 1, 0),
                new OverscaledTileID(1, 0, 1, 0, 1),
                new OverscaledTileID(1, 0, 1, 1, 1)]);

Or like whenever someone changes transform.zoom, the function returns something different for the same input.

[HarelM]

Zoom would result in a different cache key, so it's probably ok (assuming I understand what you wrote).

[wipfli]

There are only two hard things in Computer Science: cache invalidation, naming things, and off-by-one errors.

[HarelM]

Typings looks great now! A lot easier to understand the code :-) any chance you can add a unit test for this?

[wipfli]

How does a unit test for a memoized function look like? I could imagine that calling the function twice in a row with identical arguments would at least show that results are the same and should make test coverage go into both the cached and non-cached logic parts of the function.

[HarelM]

You should mock/spy something like math.abs/max and make sure it is called just for the first calculation and not for the second I guess...?

[xabbu42]

I think its wrong to add unit tests for implementation details of a function. The exact speed, if it uses caching or not and the Math functions that are used and how often in the implementation are not part of the api and therefore should not be tested. In other words, a test should only fail if we introduce a bug, not if we just choose a different but correct implementation down the road for whatever reason.

Are we sure the cache is invalidated between the single runs of the benchmark? From the pdf above my guess is yes. I also assume we have about 1ms improvement in other benchmarks but they are too slow for this to be significant.

[HarelM]

I can't disagree more. Unit test, unlike public api integration tests should check how things work internally. The question when and how the cache is invalidated should be checked by unit tests.
When talking about code coverage in relation to unit tests you can't get a high percentage without covering a lot of "ifs" which is implementation details...
That's my 2 cents at least.

[xabbu42]

Just to clarify, I was not talking about public api but about the semantics of the particular function coveringTiles. So this disagreement has nothing to to with integration vs unit testing.

We should check if the cache is invalidated correctly, that is that the cache does not introduce wrong results for any sequence of calls. If we just assume that a particular way of invalidating is correct and only test for invalidation at that points, we have no tests which could actually fail if our logic is wrong as we just repeat the logic in the tests.

The point of code coverage analysis is to find untested behaviour. You can get full code coverage without testing for implementation details assuming the function itself has no dead code. Mocking Math.abs and testing for exactly one call is similar to counting the number of if statements in a function in that context.

[HarelM]

I couldn't understand your last comment. Feel free to suggest how one should test and make sure the added code is correct and will stay correct later on.
Up until now I could only read what not to do, and not what we should do.
Looking at the benchmark results is not a valid way to tests this as it is manual and we should strive for automation. It is OK in addition to something else. :-)

[xabbu42]

I think the already added tests are sufficient. We did not treat small regressions in benchmarks as a blocker for merges in the past (see #476). If we want some automation for benchmarks, we should do that for all/most of them, but that is a separate issue.

[HarelM]

I think it would be better to split this test into several tests and have the title to be a bit more meaningful.
As a rule of thumb, a test should not have more than one assert/expect. Although I think this rule is a bit too strict and test can have a few expects, this test is not the case.

[HarelM]

I never said I wanted automation for benchmarks. This conversation is going in circles.
Feel free to approve this PR if you think it's good enough. I can't approve it without what I think are the right tests.

[ghost]

Looks like benchmarks are not affected except for LayerBackground, which runs roughly 2x faster with the caching. Is this the behavior you expected, Jannik?

I'd expect most benefit if there are many sources and much camera movement / re-rendering.
I assume the other benchmarks all share the same default sources?

Personally, after skimming over the benchmarks, I'd have expected a small difference in LayerHeatmap, LayerHillshade, LayerRaster; I don't think there are any benchmarks which focus on more complicated source setups (yet). We should probably have more benchmarks for real-world use-cases, even if we'd expect more variance in test-results due to added complexity.

So, no, not what I expected. I'm wondering if there's a bug or some other measurement issue.

However, it looks like the background layer will always do coveringTiles:

maplibre-gl-js/src/render/draw_background.ts

Line 37 in d5d4e1d

const tileIDs = transform.coveringTiles({tileSize});

, so it's possible that this particular benchmark calls it more than the others (for standard sources + then for the background).
Not sharing the same coveringTiles for all backgrounds might be considered a separate (performance-only) design-issue.

• [ ?] Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask! - I'm not sure, @JannikGM please comment about the origin of this code.

I did write this myself, but used existing mapbox 1.x code as reference (similar to surrounding code like _alignedPosMatrixCache).
Consider my personal contributions in the original PR (#105) to be in public domain / CC0, so feel free to steal it and relicense / claim as your own as you see fit.

[? ] Suggest a changelog category: bug/feature/docs/etc. or "skip changelog". @JannikGM, what would you say?

I'm not sure.

• For mapbox, the most recent similar changelog entry would be https://github.com/mapbox/mapbox-gl-js/blob/main/CHANGELOG.md#0143-feb-25-2016 (which lists something similar as "New Features & Improvements").
• For maplibre, I don't think there was a similar change yet, or it wasn't added to the changelog.
• It definitely isn't a "bug" or "docs".

[xabbu42]

I'd expect most benefit if there are many sources and much camera movement / re-rendering.
I assume the other benchmarks all share the same default sources?

yes most benchmarks use the same few default sources. in the styles benchmarks there is code which runs the same benchmark for multiple locations. would something like this help to expose the speedup of this code?

benchmarking a camera movement would be nice addition to get more relevant benchmark results, but I'm not sure how feasible it is to do that, given that the movements are done in a fixed duration. perhaps we could calculate the expected camera positions given a particular movement and a target framerate and then benchmark that succession of camera positions without any animation in between.

[ghost]

would something like this help to expose the speedup of this code?

Probably not; this optimization was meant for the case where the user has a high number of sources. I've seen maps with ~50 (!) sources (1 source-per-layer for example).
Each source manually runs the same logic to figure out which tiles are visible. This optimization adds a small cache, so sources with the same tilesize will avoid having to find all visible tiles.
So it's dependent on the amount of sources in the map, but not by how much data there is or what location it is.

benchmarking a camera movement [...] given that the movements are done in a fixed duration [...] benchmark that succession of camera positions without any animation in between.

I didn't necessarily mean an animated camera, but just tiny movements of the camera, so certain parts of the projection-logic have to be recalculated. While some benchmark test very specific parts of the code (such as symbol layout), I don't think there's anything to deal with recalculation of the matrices combined with the result effects (which includes, but is not limited to, symbol layout) ... due to camera movement. Because various effects during camera movement accumulate, it would be interesting to test these things by actually moving the camera (even if simply by having the camera jump to a new fixed location / rotation). However, benchmark strategies is a complicated topic, so it might be a separate discussion.

[xabbu42]

About the camera movement discussion: I think it should be possible to make suitable benchmarks using similar code as in bench/gl-stats.html. There a short zoom is executed as fast as possible with a hardcoded framerate by overwriting window.performance.now(). What we want here is do the same thing but instead of gathering gl stats along the way just time how fast the map can go through the "animation". Correct?

[github-actions]

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

[wipfli]

Thanks bot. I have no clue how to proceed with this...

[HarelM]

It was agreed in the steering committee that this PR will be closed.
The main benefit we believe can be achieved is by changing the algorithm of the tiles to render, this algorithm was changed a bit for the 3D terrain and we think it can be improved more and that this should be where we focus.
Closing this for now, we can always re-open it in the future.