Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance of Select Mode for MapboxGL/MaplibreGL Adapters #57

Open
JamesLMilner opened this issue Jul 2, 2023 · 1 comment
Open
Labels
MapboxGL Related to the MapboxGL JS Adapter MapLibreGL Related to the MapLibreGL JS Adapter performance An issue to do with the performance of a mode, adapter, etc

Comments

@JamesLMilner
Copy link
Owner

There have been reports (#44) that selection mode in MapLibreGL Adapter could be better.

I don't have any intimidate ideas of how to improve this as most of the performance improvement options were exhausted by #30. One option for users is to increase the dragEventThrottle up from 5 to 10/15. This may help slightly in rendering performance however it may change the UX of dragging for users (worth just experimenting and seeing what feels right).

@JamesLMilner JamesLMilner added the performance An issue to do with the performance of a mode, adapter, etc label Jul 2, 2023
@orangemug
Copy link

orangemug commented Oct 20, 2023

Hi @JamesLMilner, I had a quick look at this issue, as I was experiencing something similar in code I was writing. I think (if I'm tracing this code correctly) the setData(...) is getting behind because it's kind of a slow operation. So we can end up getting "lag" on slow devices. So essentially the following series of events ends up getting applied, even though you've already decided to update with another value.

A --> B --> C --> D --> END
A -------------------> B -------------------> C -------------------> D  -------------------> END 

What we want instead is for change B and C to be skipped, and replaced with a single update to D

A --> B --> C --> D --> END
A ---------------------> D -------------------> END   (we skipped B and C here) 

The code trail in the maplibre-gl-js code

I've had a go at writing a setDataLazy(...) helper, which appears to be helping locally. However I need to test this more and deal with edge cases / error conditions.

private async _setDataLazy(sourceId: string, newData: any) {
	const source = this._map.getSource(sourceId) as GeoJSONSource;

	if (source) {
		const queueItem = this._queue.get(source);

		if (queueItem) {
			if (queueItem.nextPromise) {
				// If we're already waiting just update the data for the next iteration + the count.
				console.log("number waiting:", queueItem.nextCount);
				this._queue.set(source, {
					...queueItem,
					nextData: newData,
					nextCount: (queueItem.nextCount ?? 0) + 1,
				});
				await queueItem.nextPromise;
			} else {
				// Unwrap a promise.
				let resolve: (value: any) => void;
				const p = new Promise<boolean>((_resolve) => (resolve = _resolve));

				this._queue.set(source, {
					...queueItem,
					nextData: newData,
					nextPromise: p,
					nextResolve: resolve!,
					nextCount: (queueItem.nextCount ?? 0) + 1,
				});
				await p;
			}
		} else {
			(source as GeoJSONSource).setData(newData);
			const p = new Promise<boolean>((resolve) => {
				this._map.on("data", (e: any) => {
					// Wait for the update to respond from the worker.
					if (
						e.dataType === "source" &&
						e.sourceDataType === "content" &&
						e.sourceId === sourceId
					) {
						resolve(true);
					}
				});
			});

			this._queue.set(source, { pendingPromise: p });
			try {
				await p;
			} catch (err: any) {
				console.error(err);
			}

			// Because we're async previously, fetch the data again to see if we have a pending update.
			const queueItemAfter = this._queue.get(source);

			this._queue.delete(source);
			if (queueItemAfter && queueItemAfter.nextResolve) {
				await this._setDataLazy(sourceId, queueItemAfter.nextData);
				queueItemAfter.nextResolve(true);
			}
		}
	}
}

There is a branch over at main...orangemug:terra-draw:fix/set-data-lazy if you want to give it a try.

@JamesLMilner JamesLMilner added MapboxGL Related to the MapboxGL JS Adapter MapLibreGL Related to the MapLibreGL JS Adapter labels Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MapboxGL Related to the MapboxGL JS Adapter MapLibreGL Related to the MapLibreGL JS Adapter performance An issue to do with the performance of a mode, adapter, etc
Projects
None yet
Development

No branches or pull requests

2 participants