Overhaul of RTL plugin loading sequence (maplibre#3728)

* fire pluginStateChange only if at least one receiver changed plugin status * draft * fixes * all ut pass * test cases * change log * worker tests * add change in style.ts * update style test * update UT * Update src/source/rtl_text_plugin_main_thread.test.ts * PR comments * PR comments * dup lazyLoad() calls * multiple * PR * replace exception with console.error * update comment to be more accurate * do not download from main thread * remove extra try/catch * remove extra throw * remove extra import * exception handling * correct version * all UT * UT * add tests (#7) * all UT * UT * Add FT * remove extra import * clean up comments * test update --------- Co-authored-by: Harel M <harel.mazor@gmail.com>
zhangyiatmicrosoft · Mar 13, 2024 · 82dce2b · 82dce2b
1 parent 136464f
commit 82dce2b
Show file tree

Hide file tree

Showing 16 changed files with 402 additions and 137 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,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
 

diff --git a/src/data/bucket/symbol_bucket.ts b/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);
                 }

diff --git a/src/index.ts b/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

diff --git a/src/source/rtl_text_plugin_main_thread.test.ts b/src/source/rtl_text_plugin_main_thread.test.ts
@@ -3,12 +3,17 @@ 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 failedToLoadMessage = `RTL Text Plugin failed to import scripts from ${url}`;
+    const SyncRTLPluginStateMessageName = 'syncRTLPluginState';
+
     beforeEach(() => {
         server = fakeServer.create();
         global.fetch = null;
@@ -17,6 +22,41 @@ describe('RTLMainThreadPlugin', () => {
         broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(() => { return Promise.resolve({} as any); });
     });
 
+    function broadcastMockSuccess(message: MessageType, payload: PluginState): Promise<PluginState[]> {
+        console.log('broadcastMockSuccessDefer', payload.pluginStatus);
+        if (message === SyncRTLPluginStateMessageName) {
+            if (payload.pluginStatus === 'loading') {
+                const resultState: PluginState = {
+                    pluginStatus: 'loaded',
+                    pluginURL: payload.pluginURL
+                };
+                return Promise.resolve([resultState]);
+            }
+        }
+    }
+
+    function broadcastMockSuccessDefer(message: MessageType, payload: PluginState): Promise<PluginState[]> {
+        if (message === SyncRTLPluginStateMessageName) {
+            if (payload.pluginStatus === 'deferred') {
+                const resultState: PluginState = {
+                    pluginStatus: 'deferred',
+                    pluginURL: payload.pluginURL
+                };
+                return Promise.resolve([resultState]);
+            }
+        }
+    }
+
+    function broadcastMockFailure(message: MessageType, payload: PluginState): Promise<PluginState[]> {
+        if (message === SyncRTLPluginStateMessageName) {
+            if (payload.pluginStatus === 'loading') {
+                return Promise.reject(failedToLoadMessage);
+            }
+        } else {
+            return Promise.resolve([]);
+        }
+    }
+
     afterEach(() => {
         server.restore();
         broadcastSpy.mockRestore();
@@ -28,56 +68,103 @@ 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);
+        const resultPromise = rtlMainThreadPlugin.setRTLTextPlugin(url);
+        await expect(resultPromise).rejects.toBe(failedToLoadMessage);
+        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
+        const deferredSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccessDefer 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(deferredSpy).toHaveBeenCalledTimes(1);
+        expect(deferredSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'deferred', pluginURL: url});
+        expect(rtlMainThreadPlugin.status).toBe('deferred');
+        deferredSpy.mockRestore();
+
+        // this is really a fire and forget
+        broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccess as any);
+        rtlMainThreadPlugin.lazyLoad();
+        await sleep(1);
+
+        // 'loading'
+        expect(broadcastSpy).toHaveBeenCalledWith(SyncRTLPluginStateMessageName, {pluginStatus: 'loading', pluginURL: url});
+        expect(broadcastSpy).toHaveBeenCalledTimes(1);
+
+        // second call to lazyLoad should not change anything
+        rtlMainThreadPlugin.lazyLoad();
+        expect(broadcastSpy).toHaveBeenCalledTimes(1);
+
+        expect(rtlMainThreadPlugin.status).toBe('loaded');
+
+        // 3rd call to lazyLoad should not change anything
+        rtlMainThreadPlugin.lazyLoad();
+        expect(rtlMainThreadPlugin.status).toBe('loaded');
+        expect(broadcastSpy).toHaveBeenCalledTimes(1);
+    });
+
+    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 be in error state if lazyLoad fails', async () => {
+        broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockSuccessDefer);
+        const resultPromise = rtlMainThreadPlugin.setRTLTextPlugin(url, true);
+        await expect(resultPromise).resolves.toBeUndefined();
+
+        expect(rtlMainThreadPlugin.status).toBe('deferred');
+
+        // the next one should fail
+        broadcastSpy = jest.spyOn(Dispatcher.prototype, 'broadcast').mockImplementation(broadcastMockFailure as any);
+
+        await expect(rtlMainThreadPlugin._requestImport()).rejects.toBe(failedToLoadMessage);
+        expect(rtlMainThreadPlugin.url).toEqual(url);
+        expect(rtlMainThreadPlugin.status).toBe('error');
     });
 });
diff --git a/src/source/rtl_text_plugin_main_thread.ts b/src/source/rtl_text_plugin_main_thread.ts
@@ -1,60 +1,79 @@
-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';
 
 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})
+            .catch((e: any) => {
+                this.status = 'error';
+                throw e;
+            });
     }
 
-    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();
-        }
-    }
 
-    async _downloadRTLTextPlugin() {
-        if (this.pluginStatus !== 'deferred' || !this.pluginURL) {
-            throw new Error('rtl-text-plugin cannot be downloaded unless a pluginURL is specified');
+        this.url = browser.resolveURL(url);
+        if (!this.url) {
+            throw new Error(`requested url ${url} is invalid`);
         }
-        try {
-            this.pluginStatus = 'loading';
-            await this._sendPluginStateToWorker();
-            await getArrayBuffer({url: this.pluginURL}, new AbortController());
-            this.pluginStatus = 'loaded';
-        } catch {
-            this.pluginStatus = 'error';
+        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();
         }
-        await this._sendPluginStateToWorker();
     }
 
-    async lazyLoadRTLTextPlugin() {
-        if (this.pluginStatus === 'deferred') {
-            await this._downloadRTLTextPlugin();
+    /** Send a message to worker which will import the RTL plugin script */
+    async _requestImport() : Promise<void> {
+
+        // all errors/exceptions will be handled by _syncState
+        await this._syncState('loading');
+        this.status = 'loaded';
+        this.fire(new Event(RTLPluginLoadedEventName));
+    }
+
+    /** Start a lazy loading process of RTL plugin */
+    lazyLoad(): void {
+        if (this.status === 'unavailable') {
+            this.status = 'requested';
+        } else if (this.status === 'deferred') {
+            this._requestImport();
         }
     }
 }