Overhaul of RTL plugin loading sequence

[zhangyiatmicrosoft]

Launch Checklist

Symptom: we were previously using 3.6.1 and recently tried to upgrade to 4.x, and noticed that map load speed is slowed down by 50ms at percentile 90. I found that it took one or two extra frames when setRTLTextPlugin is called before map creation, based on official instruction: https://maplibre.org/maplibre-gl-js/docs/examples/mapbox-gl-rtl-text/.
This problem appears to be introduced by PR #3418

The following diagrams apply when the app calls setRTLTextPlugin for the very first time and "deferred" parameter is true
3.6.1

sequenceDiagram
    participant App
    participant MapLibre Main
    participant MapLibre Worker
    App->>MapLibre Main: setRTLTextPlugin(...) "deferred" is true
    MapLibre Main->>MapLibre Main: sendPluginStateToWorker
    MapLibre Main->>MapLibre Main: Fire pluginStateChange event
    MapLibre Main->>MapLibre Main: (Style.ts) _rtlTextPluginCallback 
    MapLibre Main->>MapLibre Worker: dispatcher.broacast "syncRTLPluginState"<br /> (pluginStatus is "deferred")
    MapLibre Worker->>MapLibre Worker: syncRTLPluginState does nothing (!!!) <br /> because globalRTLTextPlugin.isLoaded() is false.

Loading

4.0.2 (after PR #3418), notice 2, 3, 4 were not there in 3.6.1
The "broadcasting-reload" cycle is done twice (loading, loaded) when lazyLoadRTLTextPlugin is called and it is downloading the plugin. Only one is enough.

sequenceDiagram
    participant App
    participant MapLibre Main
    participant MapLibre Worker
    App->>MapLibre Main: setRTLTextPlugin(...) "deferred" is true
    MapLibre Main->>MapLibre Main: 1- (rtl_text_plugin_main_thread.ts) <br/> await this._sendPluginStateToWorker
    MapLibre Main->>MapLibre Worker: dispatcher.broacast "syncRTLPluginState"<br /> (pluginStatus is "deferred")
    MapLibre Worker->>MapLibre Worker: 2- _syncRTLPluginState returns false <br /> because state.pluginStatus === 'loaded' is false.
   MapLibre Worker->>MapLibre Main:  message 
    MapLibre Main->>MapLibre Main: 3- (rtl_text_plugin_main_thread.ts) _sendPluginStateToWorker <br />fire event "pluginStateChange"
    MapLibre Main->>MapLibre Main: 4- (style.ts) _rtlTextPluginStateChange <br />reload all vector and geojson sources <br> which means ALL vector tiles !!

Loading

This PR
State diagram -- add "requested" state, so they are:
'unavailable' | 'deferred' | 'requested' | 'loading' | 'loaded' | 'error'

  graph TD;
      unavailable -- lazyLoadRTLTextPlugin --> requested;
      unavailable -- setRTLTextPlugin --> checkDefer{param<br/>defer?};
      checkDefer-- true --> deferred;
      checkDefer-- false --> loading;
      deferred -- lazyLoadRTLTextPlugin -->loading;
      requested -- setRTLTextPlugin --> loading;
      loading -- To worker and back --> checkSuccess{success?};
      checkSuccess -- true --> loaded;
      loaded --> finish[Fire event RTLPluginLoaded];
      checkSuccess -- false --> error;

Loading

Cosmetic changes to improve readability/robustness/performance:

• symbol_bucket.ts: once RTL is detect for this bucker, all future detections can be skipped

• index.ts: formatting for reading pleasure

• rtl_text_plugin_status.ts: define event name and message name to be used by other files

• worker.ts to respond with PluginState object instead of boolean, which can be too ambiguous to process

• worker_tile.ts: formatting and commenting for readability

• ajax.ts: I personally found nested tertiary operations are really hard to read and error prone. Made them to simple "if". The minimizer will do its job and end result will be the same anyway.

• Some method names are shortened

• 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.

• Include before/after visuals or gifs if this PR includes visual changes.

• Write tests for all new functionality.

• Add an entry to CHANGELOG.md under the ## main section.

[HarelM]

I'm not sure I fully understand the change in this PR.
I also am not fully sure the refactoring I did was good, but it was impossible to understand what went back from and to the worker before my changes, and now I think the code is simpler to read.
I would recommend trying to write the code related to RTL plugin better instead of patching my mistakes.
As far as I could tell, in the worker "loaded" and applyArabicShaping are equivalent as in the past setting loaded was only after this was set.
Again, the code before my changes was unreadable and the fact that you couldn't tell if you are running in the worker or the main thread was causing a headache.
So I would recommend a deeper investigation here/refactoring/design here...

[zhangyiatmicrosoft]

I'm not sure I fully understand the change in this PR. I also am not fully sure the refactoring I did was good, but it was impossible to understand what went back from and to the worker before my changes, and now I think the code is simpler to read. I would recommend trying to write the code related to RTL plugin better instead of patching my mistakes. As far as I could tell, in the worker "loaded" and applyArabicShaping are equivalent as in the past setting loaded was only after this was set. Again, the code before my changes was unreadable and the fact that you couldn't tell if you are running in the worker or the main thread was causing a headache. So I would recommend a deeper investigation here/refactoring/design here...

Understand this is a difficult PR on top of previously unreadable code (100% agree with you!)
Please try to read my before/after trace a little bit, allow me to summarize it:
before -- worker never responded.
after -- worker responds with false and "pluginStateChange" event is fired, and results in all tiles reload one extra round. Here is the callstack:

fix -- fire "pluginStateChange" only when at least one plugin state is actually changed.

[HarelM]

I see. Thanks for the info!
Any chance you could change the "before" and "after" in the initial post to use Github's Mermain markdown
I think it will help me understand the change better.
If you could create 3 charts for the following:

1. 3.6.1,
2. 4.0
3. Your proposed change

It would be super!

[zhangyiatmicrosoft]

I see. Thanks for the info! Any chance you could change the "before" and "after" in the initial post to use Github's Mermain markdown I think it will help me understand the change better. If you could create 3 charts for the following:

1. 3.6.1,
2. 4.0
3. Your proposed change

It would be super!

Very interesting tool --- did not know it exists --- quite helpful and now it is clear that my previously proposed change can be optimized further. I think 3.6.1 and old were confusing as hell because at certain times Mapbox was probably running these on both main and worker, correct?
Now if we only run in main/worker separately, a simpler and more intuitive solution can be made. Can you confirm?

[HarelM]

Yes, I agree, the original code was running in both main thread and in the worker, I have split the code to improve readability and maintainability.
Both options are valid from my point of view:

1. Don't send sync state to worker if plugin load is deferred
2. Don't raise an event of state changed if the state didn't change

Which ever you find more intuitive for you.

[zhangyiatmicrosoft]

Yes, I agree, the original code was running in both main thread and in the worker, I have split the code to improve readability and maintainability. Both options are valid from my point of view:

1. Don't send sync state to worker if plugin load is deferred
2. Don't raise an event of state changed if the state didn't change

Which ever you find more intuitive for you.

Proposed new design --- inevitably it is more complicated than I initially thought, because it must solve a race condition between Tile (calling lazyLoadRTLTextPlugin) and the hosting app (calling setRTLTextPlugin), and ensure the same outcome regardless of whichever happens first.
Previously both 3.6.1 and 4.0.2 were solving that problem by keep calling _sendPluginStateToWorker, which fires pluginStateChange event, at least 2 and up to 3 times. Each event would refresh all vector tiles and that was a performance problem.

[HarelM]

Can you draw a state machine chart with the new/updated states?
Currently there's a mix between states and method calls.
Are you suggesting to remove the loading state and add a different one called RTLRequested?

[zhangyiatmicrosoft]

Can you draw a state machine chart with the new/updated states? Currently there's a mix between states and method calls. Are you suggesting to remove the loading state and add a different one called RTLRequested?

Second thought, keep loading state is better.
Cleaned up and merged to just one diagram; hope it makes better sense now

[HarelM]

Thanks for the updated diagram!
A few comments on it:
I'm not sure I fully support sending the event only when the results from the worker arrive.
the event name is pluginStateChange and I tend to think it should be fired on every state change, at least that would make sense to me, it might be worth (if relevant) to check that we don't fire a state change if the previous state and the current one are the same, but I'm not sure it's possible right now.
As far as I understand the code and your diagram, loaded state can only be achieved when the script is fetch successfully in the main thread and loaded in the worker thread, so this probably is the right time to repaint the map - so I would consider adding this logic to style.ts.
I don't oppose to adding the requested state, it makes sense, but I think you can solve your race condition by simply calling setRTLTextPlugin before initializing the map object like it is done here:
https://maplibre.org/maplibre-gl-js/docs/examples/mapbox-gl-rtl-text/
In any case, I think we have a good solution in general, feel free to continue with implementation and tests.

[zhangyiatmicrosoft]

Thanks for the updated diagram! A few comments on it: I'm not sure I fully support sending the event only when the results from the worker arrive. the event name is pluginStateChange and I tend to think it should be fired on every state change, at least that would make sense to me, it might be worth (if relevant) to check that we don't fire a state change if the previous state and the current one are the same, but I'm not sure it's possible right now. As far as I understand the code and your diagram, loaded state can only be achieved when the script is fetch successfully in the main thread and loaded in the worker thread, so this probably is the right time to repaint the map - so I would consider adding this logic to style.ts. I don't oppose to adding the requested state, it makes sense, but I think you can solve your race condition by simply calling setRTLTextPlugin before initializing the map object like it is done here: https://maplibre.org/maplibre-gl-js/docs/examples/mapbox-gl-rtl-text/ In any case, I think we have a good solution in general, feel free to continue with implementation and tests.

• regarding 'pluginStateChange', yes the event name implies it is intended for every state change. However I feel this name was too generic to begin with. The only listener is in style ts to reload all tiles. Come to think of it, reloading tiles for state change from unavailable to deferred makes no sense and it is a waste -- and current code sends THREE rounds: deferred, loading and loaded ... I am going to change the event to be very specific 'RTLpluginLoaded'
• Not sure what do you mean by "adding this logic to style.ts.": continue from the point above, it is already there. BTW main thread does not need to fetch the script, only worker needs to import. This is another bug in existing code.
• Yes, setting setRTLTextPlugin before init will make everything work, which is what we have been doing but that has the perf problem illustrated in 4.0.2 diagram --- compared to 3.6.1, it triggers 1 or 2 extra frames and delays the map loading. Probably not many apps use RTL, and even less have concern for 50ms delay, but since we already encountered this scenario and spent so much time, might as well solve it once for all.

Draft ready. Will publish the PR and write test cases if you don't see anything fundamentally wrong.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 80.35714% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 70.90%. Comparing base (b68f6a3) to head (e49229d).
Report is 11 commits behind head on main.

Files	Patch %	Lines
src/util/ajax.ts	0.00%	8 Missing ⚠️
src/index.ts	0.00%	6 Missing ⚠️
src/source/worker.ts	83.33%	5 Missing ⚠️
src/data/bucket/symbol_bucket.ts	77.77%	0 Missing and 2 partials ⚠️
src/source/tile.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3728       +/-   ##
===========================================
- Coverage   86.59%   70.90%   -15.70%     
===========================================
  Files         240      240               
  Lines       32289    32336       +47     
  Branches     1952     1202      -750     
===========================================
- Hits        27962    22928     -5034     
- Misses       3399     8206     +4807     
- Partials      928     1202      +274     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HarelM]

Go ahead. Ping me when you set it to ready for review as GitHub doesn't notify me on this change.

[HarelM]

@zhangyiatmicrosoft any updates on this?
Do you need any help?

[zhangyiatmicrosoft]

@zhangyiatmicrosoft any updates on this? Do you need any help?
@HarelM Sorry for the delay --- took a lot of testing and debugging to make sure I did not mess up anything. Ready now.

In addition, I found that MapLibre is using strings for status everywhere, for example:
export type RTLPluginStatus = 'unavailable' | 'deferred' | 'loading' | 'loaded' | 'error'

From other code bases I learned const enum is far better, such as:
export const enum RTLPluginStatus {unavailable=0, deferred=1}
The advantages are:

• const enum is a compile time thing, and final js will be much more compact. For devs it is actually much easier to read the TS
• Common strings will not be mistaken. In this particular case, "loaded"/"loading"/"error" are all very generic and being used by tile status as well. Code searching can be very messy.

If you agree I'd like to change most of statues to const enum, one at time, starting from RTLPluginStatus in this PR

[HarelM]

Note that there are lint warnings in last commit related to unused imports, should be a few sec to fix I believe.

[zhangyiatmicrosoft]

Note that there are lint warnings in last commit related to unused imports, should be a few sec to fix I believe.

lint problem fixed

[zhangyiatmicrosoft]

I don't have write access anymore. Can some please merge. Thanks