Overhaul of RTL plugin loading sequence

CHANGELOG.md

@@ -19,6 +19,7 @@
 - Set text color to ensure contrast in the attribution pill ([3737](https://github.com/maplibre/maplibre-gl-js/pull/3737))
 - Fix memory leak in Worker when map is removed ([3734](https://github.com/maplibre/maplibre-gl-js/pull/3734))
 - Fix issue with `FullscreenControl` when MapLibre is within a [ShadowRoot](https://developer.mozilla.org/en-US/docs/Web/API/ShadowRoot) ([#3573](https://github.com/maplibre/maplibre-gl-js/pull/3573))
+- Fix performance regression with `setRTLTextPlugin` which can cause 1 or 2 extra frames to render. ([#3728](https://github.com/maplibre/maplibre-gl-js/pull/3728))
 
 ## 4.0.2
 

src/data/bucket/symbol_bucket.ts

@@ -417,7 +417,13 @@ export class SymbolBucket implements Bucket {
         this.textAnchorOffsets = new TextAnchorOffsetArray();
     }
 
-    calculateGlyphDependencies(text: string, stack: {[_: number]: boolean}, textAlongLine: boolean, allowVerticalPlacement: boolean, doesAllowVerticalWritingMode: boolean) {
+    private calculateGlyphDependencies(
+        text: string,
+        stack: {[_: number]: boolean},
+        textAlongLine: boolean,
+        allowVerticalPlacement: boolean,
+        doesAllowVerticalWritingMode: boolean) {
+
         for (let i = 0; i < text.length; i++) {
             stack[text.charCodeAt(i)] = true;
             if ((textAlongLine || allowVerticalPlacement) && doesAllowVerticalWritingMode) {
@@ -476,13 +482,13 @@ export class SymbolBucket implements Bucket {
                 // conversion here.
                 const resolvedTokens = layer.getValueAndResolveTokens('text-field', evaluationFeature, canonical, availableImages);
                 const formattedText = Formatted.factory(resolvedTokens);
-                if (containsRTLText(formattedText)) {
-                    this.hasRTLText = true;
-                }
+
+                // on this instance: if hasRTLText is already true, all future calls to containsRTLText can be skipped.
+                const bucketHasRTLText = this.hasRTLText = (this.hasRTLText || containsRTLText(formattedText));
                 if (
-                    !this.hasRTLText || // non-rtl text so can proceed safely
+                    !bucketHasRTLText || // non-rtl text so can proceed safely
                     rtlWorkerPlugin.getRTLTextPluginStatus() === 'unavailable' || // We don't intend to lazy-load the rtl text plugin, so proceed with incorrect shaping
-                    this.hasRTLText && rtlWorkerPlugin.isParsed() // Use the rtlText plugin to shape text
+                    bucketHasRTLText && rtlWorkerPlugin.isParsed() // Use the rtlText plugin to shape text
                 ) {
                     text = transformText(formattedText, layer, evaluationFeature);
                 }

src/index.ts

@@ -62,7 +62,9 @@ export type * from '@maplibre/maplibre-gl-style-spec';
  * ```
  * @see [Add support for right-to-left scripts](https://maplibre.org/maplibre-gl-js/docs/examples/mapbox-gl-rtl-text/)
  */
-function setRTLTextPlugin(pluginURL: string, lazy: boolean) { return rtlMainThreadPluginFactory().setRTLTextPlugin(pluginURL, lazy); }
+function setRTLTextPlugin(pluginURL: string, lazy: boolean): Promise<void> {
+    return rtlMainThreadPluginFactory().setRTLTextPlugin(pluginURL, lazy);
+}
 /**
  * Gets the map's [RTL text plugin](https://www.mapbox.com/mapbox-gl-js/plugins/#mapbox-gl-rtl-text) status.
  * The status can be `unavailable` (i.e. not requested or removed), `loading`, `loaded` or `error`.
@@ -73,7 +75,9 @@ function setRTLTextPlugin(pluginURL: string, lazy: boolean) { return rtlMainThre
  * const pluginStatus = getRTLTextPluginStatus();
  * ```
  */
-function getRTLTextPluginStatus() { return rtlMainThreadPluginFactory().getRTLTextPluginStatus(); }
+function getRTLTextPluginStatus(): string {
+    return rtlMainThreadPluginFactory().getRTLTextPluginStatus();
+}
 /**
  * Returns the package version of the library
  * @returns Package version of the library

src/source/rtl_text_plugin_main_thread.test.ts

@@ -3,12 +3,16 @@ import {rtlMainThreadPluginFactory} from './rtl_text_plugin_main_thread';
 import {sleep} from '../util/test/util';
 import {browser} from '../util/browser';
 import {Dispatcher} from '../util/dispatcher';
-
+import {PluginState} from './rtl_text_plugin_status';
+import {MessageType} from '../util/actor_messages';
 const rtlMainThreadPlugin = rtlMainThreadPluginFactory();
 
 describe('RTLMainThreadPlugin', () => {
     let server: FakeServer;
     let broadcastSpy: jest.SpyInstance;
+    const url = 'http://example.com/plugin';
+    const SyncRTLPluginStateMessageName = 'syncRTLPluginState';
+    let consoleSpy: jest.SpyInstance;
     beforeEach(() => {
         server = fakeServer.create();
         global.fetch = null;
@@ -17,9 +21,53 @@ describe('RTLMainThreadPlugin', () => {
         broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(() => { return Promise.resolve({} as any); });
     });
 
+    function broadcastMockSuccess(message: MessageType, payload: PluginState): Promise<PluginState[]> {
+        if (message === SyncRTLPluginStateMessageName) {
+            if (payload.pluginStatus === 'loading') {
+                const resultState: PluginState = {
+                    pluginStatus: 'loaded',
+                    pluginURL: payload.pluginURL
+                };
+                return Promise.resolve([resultState]);
+            }
+        }
+    }
+
+    function broadcastMockFailure(message: MessageType, payload: PluginState): Promise<PluginState[]> {
+        if (message === SyncRTLPluginStateMessageName) {
+            if (payload.pluginStatus === 'loading') {
+                const resultState: PluginState = {
+                    pluginStatus: 'error',
+                    pluginURL: payload.pluginURL
+                };
+                return Promise.resolve([resultState]);
+            }
+        }
+    }
+
+    /** return two results, one success one failure */
+    function broadcastMockMix(message: MessageType, payload: PluginState): Promise<PluginState[]> {
+        if (message === SyncRTLPluginStateMessageName) {
+            if (payload.pluginStatus === 'loading') {
+                const resultState0: PluginState = {
+                    pluginStatus: 'loaded',
+                    pluginURL: payload.pluginURL
+                };
+                const resultState1: PluginState = {
+                    pluginStatus: 'error',
+                    pluginURL: payload.pluginURL
+                };
+                return Promise.resolve([resultState0, resultState1]);
+            }
+        }
+    }
+
     afterEach(() => {
         server.restore();
         broadcastSpy.mockRestore();
+        if (consoleSpy) {
+            consoleSpy.mockRestore();
+        }
     });
 
     it('should get the RTL text plugin status', () => {
@@ -28,56 +76,94 @@ describe('RTLMainThreadPlugin', () => {
     });
 
     it('should set the RTL text plugin and download it', async () => {
-        const url = 'http://example.com/plugin';
-        server.respondWith(new ArrayBuffer(0));
-
-        const promise = rtlMainThreadPlugin.setRTLTextPlugin(url);
-        await sleep(0);
-        server.respond();
-        await promise;
-        expect(rtlMainThreadPlugin.pluginURL).toEqual(url);
+        broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any);
+        await rtlMainThreadPlugin.setRTLTextPlugin(url);
+        expect(rtlMainThreadPlugin.url).toEqual(url);
+        expect(rtlMainThreadPlugin.status).toBe('loaded');
     });
 
     it('should set the RTL text plugin but deffer downloading', async () => {
-        const url = 'http://example.com/plugin';
         await rtlMainThreadPlugin.setRTLTextPlugin(url, true);
-        expect(server.requests).toHaveLength(0);
-        expect(rtlMainThreadPlugin.pluginStatus).toBe('deferred');
+        expect(rtlMainThreadPlugin.status).toBe('deferred');
+        expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'deferred', pluginURL: url});
     });
 
     it('should throw if the plugin is already set', async () => {
-        const url = 'http://example.com/plugin';
         await rtlMainThreadPlugin.setRTLTextPlugin(url, true);
         await expect(rtlMainThreadPlugin.setRTLTextPlugin(url)).rejects.toThrow('setRTLTextPlugin cannot be called multiple times.');
     });
 
     it('should throw if the plugin url is not set', async () => {
         const spy = jest.spyOn(browser, 'resolveURL').mockImplementation(() => { return ''; });
-        await expect(rtlMainThreadPlugin.setRTLTextPlugin(null)).rejects.toThrow('rtl-text-plugin cannot be downloaded unless a pluginURL is specified');
+        await expect(rtlMainThreadPlugin.setRTLTextPlugin(null)).rejects.toThrow('requested url null is invalid');
         spy.mockRestore();
     });
 
     it('should be in error state if download fails', async () => {
-        const url = 'http://example.com/plugin';
-        server.respondWith(request => request.respond(404));
-        const promise = rtlMainThreadPlugin.setRTLTextPlugin(url);
-        await sleep(0);
-        server.respond();
-        await promise;
-        expect(rtlMainThreadPlugin.pluginURL).toEqual(url);
-        expect(rtlMainThreadPlugin.pluginStatus).toBe('error');
+        broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any);
+        consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
+        await rtlMainThreadPlugin.setRTLTextPlugin(url);
+        expect(consoleSpy).toHaveBeenCalledWith(`worker failed to load ${url}, worker status is error`);
+        expect(rtlMainThreadPlugin.url).toEqual(url);
+        expect(rtlMainThreadPlugin.status).toBe('error');
     });
 
-    it('should lazy load the plugin if deffered', async () => {
-        const url = 'http://example.com/plugin';
-        server.respondWith(new ArrayBuffer(0));
+    it('should lazy load the plugin if deferred', async () => {
+        // use success spy to make sure test case does not throw exception
+        broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any);
         await rtlMainThreadPlugin.setRTLTextPlugin(url, true);
-        expect(server.requests).toHaveLength(0);
-        expect(rtlMainThreadPlugin.pluginStatus).toBe('deferred');
-        const promise = rtlMainThreadPlugin.lazyLoadRTLTextPlugin();
-        await sleep(0);
-        server.respond();
-        await promise;
-        expect(rtlMainThreadPlugin.pluginStatus).toBe('loaded');
+        expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'deferred', pluginURL: url});
+        expect(rtlMainThreadPlugin.status).toBe('deferred');
+
+        // this is really a fire and forget
+        // two calls to lazyLoad
+        rtlMainThreadPlugin.lazyLoad();
+        await sleep(1);
+        expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url});
+
+        // two times, first for "deferred", second for 'loading'
+        expect(broadcastSpy).toHaveBeenCalledTimes(2);
+
+        // second call to lazyLoad should not change anything
+        rtlMainThreadPlugin.lazyLoad();
+        expect(broadcastSpy).toHaveBeenCalledTimes(2);
+
+        expect(rtlMainThreadPlugin.status).toBe('loaded');
+
+        // 3rd call to lazyLoad should not change anything
+        rtlMainThreadPlugin.lazyLoad();
+        expect(rtlMainThreadPlugin.status).toBe('loaded');
+        expect(broadcastSpy).toHaveBeenCalledTimes(2);
+    });
+
+    it('should set status to requested if RTL plugin was not set', async () => {
+        rtlMainThreadPlugin.lazyLoad();
+        expect(rtlMainThreadPlugin.status).toBe('requested');
+    });
+
+    it('should immediately download if RTL plugin was already requested, ignoring deferred:true', async () => {
+        broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any);
+        rtlMainThreadPlugin.lazyLoad();
+        expect(rtlMainThreadPlugin.status).toBe('requested');
+        await sleep(1);
+
+        // notice even when deferred is true, it should download because already requested
+        await rtlMainThreadPlugin.setRTLTextPlugin(url, true);
+        expect(rtlMainThreadPlugin.status).toBe('loaded');
+        expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url});
+    });
+
+    it('should allow multiple calls to lazyLoad', async () => {
+        rtlMainThreadPlugin.lazyLoad();
+        expect(rtlMainThreadPlugin.status).toBe('requested');
+        rtlMainThreadPlugin.lazyLoad();
+        expect(rtlMainThreadPlugin.status).toBe('requested');
+    });
+
+    it('should report error for multiple results and one failure', async () => {
+        broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockMix as any);
+        consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
+        await rtlMainThreadPlugin.setRTLTextPlugin(url);
+        expect(consoleSpy).toHaveBeenCalledWith(`worker failed to load ${url}, worker status is error`);
     });
 });

src/source/rtl_text_plugin_main_thread.ts

@@ -1,60 +1,102 @@
-import {getArrayBuffer} from '../util/ajax';
+
 import {browser} from '../util/browser';
 import {Event, Evented} from '../util/evented';
-import {RTLPluginStatus, PluginState} from './rtl_text_plugin_status';
+import {RTLPluginStatus, RTLPluginLoadedEventName, PluginState} from './rtl_text_plugin_status';
 import {Dispatcher, getGlobalDispatcher} from '../util/dispatcher';
 
+import {getArrayBuffer} from '../util/ajax';
+import {WorkerPool} from '../util/worker_pool';
+
 class RTLMainThreadPlugin extends Evented {
-    pluginStatus: RTLPluginStatus = 'unavailable';
-    pluginURL: string = null;
+    status: RTLPluginStatus = 'unavailable';
+    url: string = null;
     dispatcher: Dispatcher = getGlobalDispatcher();
-    queue: PluginState[] = [];
 
-    async _sendPluginStateToWorker() {
-        await this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: this.pluginStatus, pluginURL: this.pluginURL});
-        this.fire(new Event('pluginStateChange', {pluginStatus: this.pluginStatus, pluginURL: this.pluginURL}));
+    /** Sync RTL plugin state by broadcasting a message to the worker */
+    _syncState(statusToSend: RTLPluginStatus): Promise<PluginState[]> {
+        this.status = statusToSend;
+        return this.dispatcher.broadcast('syncRTLPluginState', {pluginStatus: statusToSend, pluginURL: this.url});
     }
 
-    getRTLTextPluginStatus() {
-        return this.pluginStatus;
+    /** This one is exposed to outside */
+    getRTLTextPluginStatus(): RTLPluginStatus {
+        return this.status;
     }
 
-    clearRTLTextPlugin() {
-        this.pluginStatus = 'unavailable';
-        this.pluginURL = null;
+    clearRTLTextPlugin(): void {
+        this.status = 'unavailable';
+        this.url = null;
     }
 
     async setRTLTextPlugin(url: string, deferred: boolean = false): Promise<void> {
-        if (this.pluginStatus === 'deferred' || this.pluginStatus === 'loading' || this.pluginStatus === 'loaded') {
+        if (this.url) {
+            // error
             throw new Error('setRTLTextPlugin cannot be called multiple times.');
         }
-        this.pluginURL = browser.resolveURL(url);
-        this.pluginStatus = 'deferred';
-        await this._sendPluginStateToWorker();
-        if (!deferred) {
-            //Start downloading the plugin immediately if not intending to lazy-load
-            await this._downloadRTLTextPlugin();
+
+        this.url = browser.resolveURL(url);
+        if (!this.url) {
+            throw new Error(`requested url ${url} is invalid`);
+        }
+        if (this.status === 'unavailable') {
+
+            // from initial state:
+            if (deferred) {
+
+                this.status = 'deferred';
+                // fire and forget: in this case it does not need wait for the broadcasting result
+                // it is important to sync the deferred status once because
+                // symbol_bucket will be checking it in worker
+                this._syncState(this.status);
+
+            } else {
+                return this._requestImport();
+            }
+
+        } else if (this.status === 'requested') {
+            return this._requestImport();
         }
     }
 
-    async _downloadRTLTextPlugin() {
-        if (this.pluginStatus !== 'deferred' || !this.pluginURL) {
-            throw new Error('rtl-text-plugin cannot be downloaded unless a pluginURL is specified');
+    /** Send a message to worker which will import the RTL plugin script */
+    async _requestImport() : Promise<void> {
+
+        // Reference PR: https://github.com/mapbox/mapbox-gl-js/pull/9122
+        // if have more than 1 workers, it is better to load once in main thread to warm up browser cache
+        // so all workers can use it --- even though the result of getArrayBuffer is not being used here.
+        // Otherwise, just let worker importScript once.
+        if (WorkerPool.workerCount > 1) {
+            await getArrayBuffer({url: this.url}, new AbortController());
         }
+
         try {
-            this.pluginStatus = 'loading';
-            await this._sendPluginStateToWorker();
-            await getArrayBuffer({url: this.pluginURL}, new AbortController());
-            this.pluginStatus = 'loaded';
-        } catch {
-            this.pluginStatus = 'error';
+            const workerResults = await this._syncState('loading');
+
+            // expect all of them to be 'loaded'
+            const expectedStatus = 'loaded';
+            const failedToLoadWorkers = workerResults.filter((workerResult) => {
+                return workerResult.pluginStatus !== expectedStatus;
+            });
+
+            if (failedToLoadWorkers.length > 0) {
+                throw failedToLoadWorkers[0].pluginStatus;
+            } else {
+                // all success
+                this.status = expectedStatus;
+                this.fire(new Event(RTLPluginLoadedEventName));
+            }
+        } catch (e) {
+            this.status = 'error';
+            console.error(`worker failed to load ${this.url}, worker status is ${e.toString()}`);
         }
-        await this._sendPluginStateToWorker();
     }
 
-    async lazyLoadRTLTextPlugin() {
-        if (this.pluginStatus === 'deferred') {
-            await this._downloadRTLTextPlugin();
+    /** Start a lazy loading process of RTL plugin */
+    lazyLoad(): void {
+        if (this.status === 'unavailable') {
+            this.status = 'requested';
+        } else if (this.status === 'deferred') {
+            this._requestImport();
         }
     }
 }