VectorTileWorkerSource: change reloadTile behavior to reload right away instead of waiting for previous parse to complete

[ambientlight]

As pointed out in #1805 (comment), the issue occurred where after adding a geojson source, a bunch of symbol layers and new spritesheets in a load callback, the sprites would be not shown for geojson's source tiles already loaded (which are loaded before the sprites are loaded), as a result of tiles not being reparsed:

maplibre-gl-js/src/source/worker_tile.ts

Line 64 in 870c2a3

parse(data: VectorTile, layerIndex: StyleLayerIndex, availableImages: Array<string>, actor: Actor, callback: WorkerTileCallback) {

not getting called inside the web-worker and so the

maplibre-gl-js/src/source/worker_tile.ts

Line 169 in 870c2a3

actor.send('getImages', {icons: patterns, source: this.source, tileID: this.tileID, type: 'patterns'}, (err, result) => {

would not fire to grab the new loaded image assets.

This issue would not happen if browser have cached sprite assets, hinting on some async behavior weirdness. Tracing the flow of

maplibre-gl-js/src/style/style.ts

Line 503 in 870c2a3

_updateTilesForChangedImages() {

happened to show that

maplibre-gl-js/src/source/vector_tile_worker_source.ts

Lines 157 to 167 in 870c2a3

	const done = (err?: Error, data?: any) => {
	const reloadCallback = workerTile.reloadCallback;
	if (reloadCallback) {
	delete workerTile.reloadCallback;
	workerTile.parse(workerTile.vectorTile, vtSource.layerIndex, this.availableImages, vtSource.actor, reloadCallback);
	}
	callback(err, data);
	};
	if (workerTile.status === 'parsing') {
	workerTile.reloadCallback = done;

done callback that would perform reparsing and would be set if the workerTile is already in the parsing state would not fire. This happened because the workerTile.reloadCallback would only happen if the workerTile.parse (that sets the status to parsing) would itself be called from reloadTile, but would not from its own loadTile logic that could happen prior to this reloadTile call.

This PR fixes that by checking the reloadCallback inside the loadTile's workerTile.parse callback.

UPDATE: 2023/07/15

reload callback has been eliminated per conversation in favor of reloading right away and cancelling in flight dependency sprite / glyphs dependency loading during parse at https://github.com/maplibre/maplibre-gl-js/blob/main/src/source/worker_tile.ts#L141:L177.

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.
• Write tests for all new functionality.
• Add an entry to CHANGELOG.md under the ## main section.

[github-actions]

Bundle size report:

Size Change: +17 B
Total Size Before: 207 kB
Total Size After: 207 kB

Output file	Before	After	Change
maplibre-gl.js	198 kB	198 kB	+17 B
maplibre-gl.css	9.1 kB	9.1 kB	0 B

ℹ️ View Details

Source file	Before	After	Change
src/source/geojson_wrapper.ts	405 B	452 B	+47 B
src/source/vector_tile_worker_source.ts	698 B	722 B	+24 B
src/render/program/program_uniforms.ts	945 B	958 B	+13 B
src/source/geojson_source_diff.ts	609 B	616 B	+7 B
src/render/program/debug_program.ts	188 B	195 B	+7 B
src/render/draw_debug.ts	1.37 kB	1.37 kB	+7 B
src/ui/handler/tap_recognizer.ts	529 B	535 B	+6 B
node_modules/gl-matrix/esm/vec3.js	805 B	810 B	+5 B
src/data/pos_attributes.ts	80 B	85 B	+5 B
src/render/program/hillshade_program.ts	881 B	886 B	+5 B
src/render/draw_hillshade.ts	1.15 kB	1.16 kB	+5 B
src/render/draw_symbol.ts	2.6 kB	2.6 kB	+4 B
src/render/draw_terrain.ts	1.75 kB	1.75 kB	+4 B
src/ui/default_locale.ts	309 B	313 B	+4 B
node_modules/@mapbox/geojson-rewind/index.js	339 B	342 B	+3 B
src/util/offscreen_canvas_supported.ts	153 B	156 B	+3 B
src/source/image_source.ts	1.14 kB	1.14 kB	+3 B
src/util/web_worker.ts	39 B	42 B	+3 B
src/render/draw_fill.ts	972 B	975 B	+3 B
src/render/draw_circle.ts	619 B	622 B	+3 B
src/ui/handler/tap_zoom.ts	366 B	369 B	+3 B
src/source/worker_tile.ts	1.35 kB	1.35 kB	+2 B
src/util/global_worker_pool.ts	330 B	332 B	+2 B
src/shaders/background_pattern.vertex.glsl.g.ts	224 B	226 B	+2 B
src/shaders/clipping_mask.fragment.glsl.g.ts	59 B	61 B	+2 B
src/shaders/heatmap.vertex.glsl.g.ts	366 B	368 B	+2 B
src/shaders/fill_outline.vertex.glsl.g.ts	216 B	218 B	+2 B
src/shaders/fill_pattern.fragment.glsl.g.ts	394 B	396 B	+2 B
src/shaders/hillshade_prepare.vertex.glsl.g.ts	190 B	192 B	+2 B
src/shaders/line_pattern.vertex.glsl.g.ts	787 B	789 B	+2 B
src/shaders/symbol_sdf.fragment.glsl.g.ts	552 B	554 B	+2 B
src/shaders/symbol_text_and_icon.vertex.glsl.g.ts	1.01 kB	1.01 kB	+2 B
src/render/program/terrain_program.ts	700 B	702 B	+2 B
src/render/draw_collision_debug.ts	1.14 kB	1.14 kB	+2 B
node_modules/gl-matrix/esm/mat2.js	227 B	229 B	+2 B
src/ui/handler/mouse.ts	522 B	524 B	+2 B
src/ui/camera.ts	3.4 kB	3.41 kB	+2 B
node_modules/geojson-vt/src/convert.js	859 B	860 B	+1 B
node_modules/geojson-vt/src/index.js	1.56 kB	1.56 kB	+1 B
src/util/dispatcher.ts	358 B	359 B	+1 B
src/symbol/projection.ts	1.82 kB	1.82 kB	+1 B
src/style/pauseable_placement.ts	598 B	599 B	+1 B
src/style/style.ts	7.37 kB	7.37 kB	+1 B
src/style-spec/empty.ts	155 B	156 B	+1 B
src/shaders/heatmap_texture.vertex.glsl.g.ts	144 B	145 B	+1 B
src/shaders/fill.fragment.glsl.g.ts	177 B	178 B	+1 B
src/shaders/hillshade.vertex.glsl.g.ts	138 B	139 B	+1 B
src/shaders/line.fragment.glsl.g.ts	324 B	325 B	+1 B
src/shaders/terrain_coords.fragment.glsl.g.ts	169 B	170 B	+1 B
src/gl/index_buffer.ts	347 B	348 B	+1 B
src/gl/vertex_buffer.ts	609 B	610 B	+1 B
src/gl/color_mode.ts	172 B	173 B	+1 B
src/gl/depth_mode.ts	132 B	133 B	+1 B
src/gl/stencil_mode.ts	153 B	154 B	+1 B
src/ui/events.ts	356 B	357 B	+1 B
src/ui/handler/drag_move_state_manager.ts	336 B	337 B	+1 B
src/ui/handler/two_fingers_touch.ts	1.03 kB	1.03 kB	+1 B
src/ui/handler/keyboard.ts	571 B	572 B	+1 B
src/ui/handler/shim/dblclick_zoom.ts	149 B	150 B	+1 B
src/ui/handler/shim/two_fingers_touch.ts	280 B	281 B	+1 B
node_modules/@mapbox/tiny-sdf/index.js	1.1 kB	1.1 kB	-1 B
src/source/tile_bounds.ts	317 B	316 B	-1 B
src/util/worker_pool.ts	419 B	418 B	-1 B
src/symbol/path_interpolator.ts	311 B	310 B	-1 B
src/source/pixels_to_tile_units.ts	104 B	103 B	-1 B
src/shaders/background_pattern.fragment.glsl.g.ts	296 B	295 B	-1 B
src/shaders/fill_outline.fragment.glsl.g.ts	254 B	253 B	-1 B
src/render/program/background_program.ts	473 B	472 B	-1 B
src/render/draw_background.ts	579 B	578 B	-1 B
src/ui/handler/box_zoom.ts	706 B	705 B	-1 B
src/ui/handler/touch_pan.ts	540 B	539 B	-1 B
src/ui/handler/scroll_zoom.ts	1.28 kB	1.28 kB	-1 B
src/util/debug.ts	161 B	160 B	-1 B
src/ui/control/logo_control.ts	486 B	485 B	-1 B
src/ui/control/geolocate_control.ts	2.24 kB	2.24 kB	-1 B
src/render/glyph_atlas.ts	330 B	328 B	-2 B
src/source/raster_tile_source.ts	912 B	910 B	-2 B
src/source/geojson_source.ts	1.37 kB	1.37 kB	-2 B
src/data/raster_bounds_attributes.ts	96 B	94 B	-2 B
src/style-spec/diff.ts	1.53 kB	1.53 kB	-2 B
src/shaders/heatmap_texture.fragment.glsl.g.ts	207 B	205 B	-2 B
src/shaders/debug.vertex.glsl.g.ts	163 B	161 B	-2 B
src/shaders/fill_pattern.vertex.glsl.g.ts	389 B	387 B	-2 B
src/shaders/hillshade.fragment.glsl.g.ts	555 B	553 B	-2 B
src/shaders/line.vertex.glsl.g.ts	709 B	707 B	-2 B
src/shaders/line_pattern.fragment.glsl.g.ts	707 B	705 B	-2 B
src/shaders/symbol_sdf.vertex.glsl.g.ts	1.01 kB	1.01 kB	-2 B
src/shaders/terrain.fragment.glsl.g.ts	112 B	110 B	-2 B
src/ui/hash.ts	939 B	937 B	-2 B
src/data/pos3d_attributes.ts	88 B	86 B	-2 B
src/shaders/terrain.vertex.glsl.g.ts	221 B	218 B	-3 B
src/shaders/shaders.ts	1.49 kB	1.49 kB	-3 B
src/shaders/clipping_mask.vertex.glsl.g.ts	106 B	103 B	-3 B
node_modules/gl-matrix/esm/mat3.js	224 B	221 B	-3 B
src/render/program/clipping_mask_program.ts	110 B	107 B	-3 B
src/render/program/heatmap_program.ts	560 B	557 B	-3 B
src/util/primitives.ts	1.01 kB	1.01 kB	-3 B
src/ui/handler/map_event.ts	446 B	443 B	-3 B
src/ui/handler/drag_handler.ts	499 B	496 B	-3 B
src/source/raster_dem_tile_worker_source.ts	391 B	387 B	-4 B
src/source/raster_dem_tile_source.ts	972 B	968 B	-4 B
src/render/program/fill_extrusion_program.ts	795 B	791 B	-4 B
src/render/program/raster_program.ts	563 B	559 B	-4 B
src/gl/context.ts	1.29 kB	1.28 kB	-4 B
src/render/draw_line.ts	1.05 kB	1.04 kB	-4 B
src/ui/handler/handler_util.ts	178 B	174 B	-4 B
src/render/program/symbol_program.ts	1.3 kB	1.29 kB	-6 B
src/render/painter.ts	3.76 kB	3.75 kB	-6 B
src/render/draw_heatmap.ts	1.04 kB	1.04 kB	-7 B
src/render/draw_fill_extrusion.ts	796 B	788 B	-8 B
src/source/geojson_worker_source.ts	1.74 kB	1.74 kB	-9 B
src/render/draw_raster.ts	1.05 kB	1.04 kB	-9 B
node_modules/vt-pbf/lib/geojson_wrapper.js	479 B	449 B	-30 B

[HarelM]

Thanks for taking the time to fix this!!
Can you please add tests and change log entry?

[ambientlight]

@HarelM: done, thanks a lot!

[HarelM]

@ambientlight thanks! see me new comments. Sorry for the ping-pong...

[ambientlight]

@HarelM: thanks for the review, let me know if all your questions are addressed now

[HarelM]

@ambientlight do you want to push thie PR forward or close it?
Last code review thread is talking about a small redesign of this solution.

[ambientlight]

@HarelM: yes, thanks for reminding this

[HarelM]

Please let me know when this is ready to review again.

[ambientlight]

@HarelM: I am still on this, but I am having difficulties figuring out a consistent repro again, I can only catch the original issue with sprites not loading if I use custom images via map.loadImage() in a very rare circumstance, like hitting 50 browser reloads to catch this...
@smellyshovel: by any chance, do you still have an example from #1805 (comment) ?

MapLibre.GL.JS.debug.page.mp4

I guess this is not really a blocker unless we need a render test here

[smellyshovel]

@ambientlight @HarelM Hello to everyone. Sorry for slipping out of this, got distracted with other things to do, then vacation and so on and so on...
I'm gonna go through all the comments here and for the other issue (something about style.load event I guess?) hopefully today by the evening and will provide you with anything I'll be able to figure out (if anything).

[ambientlight]

@smellyshovel: nothing on your end actually, I was just curious if by some chance you still have the repro siting somewhere from the PR you have completed last year (#1805 (comment))

[smellyshovel]

@ambientlight then I'm afraid I won't be of much help here. Definitely don't have anything from half a year ago on my machine now :(

[smellyshovel]

@ambientlight on the other hand I remember roughly the state of the project when I took the video you mentioned in the OP.
It was at the moment when I implemented the support for multiple sprites, but haven't touched any of the rendering/tiles-updating code. So, just as an idea, you may want to take the code before my PR, add parts responsible for supporting multiple sprites and then try to make a repro out of there. But I can imagine how much hussle it is, so probably not a realistic solution.

UPD.
3fe6f66
This commit might be what you're looking for. No repro back from that time is present anymore though...

[ambientlight]

@smellyshovel: thanks a lot, I found the demo page you used in your video in that commit, (though the repro is same flaky)

[ambientlight]

@HarelM: after investigating further, I don't think there is anything to add to this PR actually. For context: we have discussed the issue where when multiple reloadTile is send to the webworker while the tile of interest is in progress of being parsed, reloadCallback inside workerTile will be overridden, thus not all reloadTile will be evented back on the main thread.
The reloadTiles on the main thread are triggered at

maplibre-gl-js/src/source/geojson_source.ts

Line 404 in 2a63bff

tile.request = this.actor.send(message, params, (err, data) => {

and

maplibre-gl-js/src/source/vector_tile_source.ts

Line 213 in 2a63bff

tile.request = tile.actor.send('reloadTile', params, done.bind(this));

with reloadTile callback eventually hitting

maplibre-gl-js/src/source/source_cache.ts

Line 275 in 2a63bff

_tileLoaded(tile: Tile, id: string, previousState: TileState, err?: Error | null) {

If I understand this correctly, in this case the lost callback inside web worker doesn't cause the problem on the main thread since the last callback will hit the eventual sourceCache._tileLoaded for the same tile.

If we would actually event on all callbacks like:

/**
 * Parses worker tile and invokes all pending reload callbacks.
 */
_parseTileAndConsumeReloadCallbacks(workerTile: WorkerTile) {
    const reloadCallbacks = workerTile.reloadCallbacks;
    if (reloadCallbacks) {
        delete workerTile.reloadCallbacks;
        workerTile.parse(workerTile.vectorTile, this.layerIndex, this.availableImages, this.actor, (err, data: WorkerTileResult) => {
            for (const reloadCallback of reloadCallbacks) {
                reloadCallback(err, data);
            }   
        });
    }
}

we would run into problem, since we would need to copy the data returned, since the ownership of ArrayBuffers, etc. in WorkerTileResult will be transferred from web worker to main thread and so no longer accessible in web worker.

Let me know what you think.

[HarelM]

The last comment I wrote here: #1874 (comment) was talking about moving some logic into the worker tile class.
This is mainly about the management of the callback.
Your last comment doesn't answer that as far as I understand.
Let's try to close this before I need to read all the comments again in order to remember what we talked about...

[codecov-commenter]

Codecov Report

Patch coverage: 82.35% and project coverage change: +0.50 🎉

Comparison is base (541ac26) 73.97% compared to head (3478963) 74.48%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1874      +/-   ##
==========================================
+ Coverage   73.97%   74.48%   +0.50%     
==========================================
  Files         238      238              
  Lines       18996    19002       +6     
  Branches     4281     4284       +3     
==========================================
+ Hits        14052    14153     +101     
+ Misses       4944     4849      -95     

Impacted Files	Coverage Δ
src/source/worker_tile.ts	95.20% <78.57%> (+14.49%)	⬆️
src/source/vector_tile_worker_source.ts	96.15% <100.00%> (-0.28%)	⬇️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ambientlight]

@HarelM: ou, missed that, let me clarify something first:

I would consider moving some of the logic into the workerTile class somehow, as you suggested.

So my prior comment there was not exactly about the callbacks but the fact that when reloadCallback is set, we essentially wait for dependencies at https://github.com/maplibre/maplibre-gl-js/blob/main/src/source/worker_tile.ts#L141:L177 to fetch, perform maybePrepare and then we reparse again.

Maybe there was a reason for that (like not doing it aggressively on every reloadTile call, but instead swapping callbacks and doing it once in the end), but let me check what if we reparse right away and cancel in-flight getGlyphs / getImages so they don't call maybePrepare. (this will eliminate reloadCallback)

[HarelM]

Ok, waiting on your check in order to merge this.

[ambientlight]

@HarelM: pushed this 'idea', let me know what you think here.

note, we may incur higher loads in web worker, if reloadTile are being requested aggressively (while tile is already waiting for dependency load in parseTile) since now we actually do the reload right away on every invocation, but we may gain faster tile reload in the end since we do not wait for (outdated) dependencies to fetch before the reload.

[HarelM]

I think this looks good in general - the worker tile is self contained generally.
Any chance you can increase the coverage of worker_tile.ts class? I see that the coverage isn't great...

[ambientlight]

@HarelM: done, thanks.

[HarelM]

THANKS!!