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

Add option to load rtl-text-plugin lazily #8865

Merged
merged 14 commits into from
Oct 24, 2019
Merged

Conversation

arindam1993
Copy link
Contributor

@arindam1993 arindam1993 commented Oct 12, 2019

Addresses #8825.
Adds an rtlTextPluginURL option to the map, and map will load the rtlTextPlugin the first time rtl text is encountered

Adds a new lazy parameter to setRTLTextPlugin(url: string, cb: Function, lazy: boolean). Its false by default to keep existing behavior, but when set to true, it will cause mapboxgl to lazily load the rtl-text-plugin the first time it encounters rtl-text, it also prevents the rtl-text from rendering until the plugin is downloaded and parsed after which it loads in.

This includes a few more enhancements,

  • It will now make the network request for the plugin only once, and then distribute the downloaded data to all workers via a blobUrl
  • the callback function is now invoked only once when the plugin is parsed and ready on all workers, instead of being repeatedly invoked for each worker.

Under the hood, theres a re-write of how the main-thread -> worker communication for the plugin workers. The main thread now has full ownership of the state of the plugin and pushes it to the workers. The workers are reactive and download and parse the plugin once all the data is available.

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

@arindam1993 arindam1993 added this to the release-sangria milestone Oct 14, 2019
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arindam1993 Can you describe the expected behavior when using this map options?

What I am seeing when loading this URL: http://localhost:9966/debug/rtl-plugin-autoload.html#12.22/27.34633/45.00641, with throttled network or CPU performance (from Chrome dev tools), is that the Arabic text shows up left-to-right, and flashes before being re-rendered correctly.

Would it be possible to hold off that text from being rendered so that it fades in after the plugin is loaded?

Additionally, this API only works with a URL to a separately hosted rtl-text-plugin. This means that developers who package the plugin, or bundle GL-JS differently will not be able to load this plugin on-demand. (cc @kappaxbeta @patfrat)

There's a fair bit of overlap between this PR and #8864. It may make sense to commit #8664 first and then rebase this on top of that so that it can take advantage of the loading/requested state. In my debugging, the plugin was being fetched twice, based on the Devtools network tab.
cc @ansis

@arindam1993
Copy link
Contributor Author

What I am seeing when loading this URL: http://localhost:9966/debug/rtl-plugin-autoload.html#12.22/27.34633/45.00641, with throttled network or CPU performance (from Chrome dev tools), is that the Arabic text shows up left-to-right, and flashes before being re-rendered correctly.

Would it be possible to hold off that text from being rendered so that it fades in after the plugin is loaded?

Yes, the expected behaviour should be, all LTR text loads as expected, and RTL text loads appears after a slight delay once the plugin finishes downloading.

Additionally, this API only works with a URL to a separately hosted rtl-text-plugin. This means that developers who package the plugin, or bundle GL-JS differently will not be able to load this plugin on-demand. (cc @kappaxbeta @patfrat)

I think this PR shouldn't try to rework the delivery process, but we can make the loading lazy and slightly more efficient.

There's a fair bit of overlap between this PR and #8864. It may make sense to commit #8664 first and then rebase this on top of that so that it can take advantage of the loading/requested state. In my debugging, the plugin was being fetched twice, based on the Devtools network tab.
cc @ansis

Yes, will wait for #8864 to be ready, the plugin being fetched multiple times is the current status-quo though, since each worker requests for the plugin separately. I'll change that in this PR so that the main thread downloads the plugin and then the workers load it via blob url.

@@ -413,7 +414,9 @@ class SymbolBucket implements Bucket {
if (formattedText.containsRTLText()) {
this.hasRTLText = true;
}
text = transformText(formattedText, layer, feature);
if (this.hasRTLText && globalRTLTextPlugin.isLoaded() || !this.hasRTLText) {
text = transformText(formattedText, layer, feature);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a warning emitted here so that customers don't just see a map with no labels and wonder what the heck is going on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the behavior now so that only when lazy loading, does it skip rendering the text.

…ired-plugin

# Conflicts:
#	src/source/rtl_text_plugin.js
@arindam1993 arindam1993 changed the title Load rtlTextPlugin on-demand using rtlTextPluginURL map option [dnm]Load rtlTextPlugin on-demand using rtlTextPluginURL map option Oct 15, 2019
@arindam1993 arindam1993 removed this from the release-sangria milestone Oct 16, 2019
unavailable: 'unavailable', // Not loaded
available: 'available', // Host url specified, but we havent started loading yet
loading: 'loading', // request in-flight
downloaded: 'downloaded', //plugin loaded and cached on main-thread and pluginBlobURL for worker is generated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between downloaded and loaded, and why would we want to expose that distinction?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally don't think these statuses make intuitive sense. We shouldn't be asking end users to understand the intricacies of lazy loading the RTL plugin on the main thread and worker threads in order to get a status update. I hated unavailable when I included it but couldn't think of a better option. available meaning that the user has just specified a URL isn't intuitive. And I agree that downloaded probably shouldn't be exposed at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, I've changed the architecture as described in the main PR body.

src/ui/map.js Outdated
@@ -278,6 +278,7 @@ class Map extends Camera {
_mapId: number;
_localIdeographFontFamily: string;
_requestManager: RequestManager;
_rtlTextPluginURL: ?string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed right?

Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think overall this looks fine. It's a little tough to follow the code pathway but that's probably a natural consequence of having to pipe some of this information around the code base. My biggest concern is probably the various statuses (their naming and the states they represent).

Also, we need to make sure we have good documentation on the expected behavior of this plugin now.


if (
this.hasRTLText && globalRTLTextPlugin.isLoaded() || // Use the rtlText plugin shape text
!this.hasRTLText || // non-rtl terxt so can proceed safely
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: terxt

if (
this.hasRTLText && globalRTLTextPlugin.isLoaded() || // Use the rtlText plugin shape text
!this.hasRTLText || // non-rtl terxt so can proceed safely
!globalRTLTextPlugin.isAvailableInWorker() // We-doent intend to async-load the rtl text plugin, so proceed with incorrect shaping
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We-doent


if (
this.hasRTLText && globalRTLTextPlugin.isLoaded() || // Use the rtlText plugin shape text
!this.hasRTLText || // non-rtl terxt so can proceed safely
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is minor, but it seems like the order here should be !this.hasRTLText || !globalRTLTextPlugin.isAvailableInWorker() || this.hasRTLText && globalRTLTextPlugin.isLoaded(). Start with the most generic check and work your way down to more complex ones.

<script src='../dist/mapbox-gl-dev.js'></script>
<script src='../debug/access_token_generated.js'></script>
<script>
mapboxgl.setRTLTextPlugin('https://api.mapbox.com/mapbox-gl-js/plugins/mapbox-gl-rtl-text/v0.2.3/mapbox-gl-rtl-text.js', null, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the expectation here is that the plugin won't be loaded right away? How would we see that? Can this be modified to fly to somewhere with RTL text label names and emit an alert when the popup is loaded so we can see it working? And can this be combined with the existing rtl.html debug page?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made the plugin loading interactive in the rtl.html page, so you can click a button and see the status live.

unavailable: 'unavailable', // Not loaded
available: 'available', // Host url specified, but we havent started loading yet
loading: 'loading', // request in-flight
downloaded: 'downloaded', //plugin loaded and cached on main-thread and pluginBlobURL for worker is generated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally don't think these statuses make intuitive sense. We shouldn't be asking end users to understand the intricacies of lazy loading the RTL plugin on the main thread and worker threads in order to get a status update. I hated unavailable when I included it but couldn't think of a better option. available meaning that the user has just specified a URL isn't intuitive. And I agree that downloaded probably shouldn't be exposed at all.

return callback;
};

export const clearRTLTextPlugin = function() {
pluginStatus = status.unavailable;
pluginURL = null;
pluginBlobURL = null;
lazy = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if lazy is a boolean, shouldn't it default to false?

pluginURL = browser.resolveURL(url);
lazy = !!lazyLoad;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, if we just default to false and always treated it as a boolean, we wouldn't have to do the cast here.

throw error;
} else {
const rtlBlob = new window.Blob([data], {type: 'application/javascript'});
const URL = window.URL || window.webkitURL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need webkitURL here? i can't even really find much information on it but it seems to be deprecated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may have been added in the past when Safari/Webkit didn't support window.URL. But that doesn't seem to be an issue any longer based on caniuse

src/util/script_detection.js Show resolved Hide resolved
@arindam1993
Copy link
Contributor Author

I think overall this looks fine. It's a little tough to follow the code pathway but that's probably a natural consequence of having to pipe some of this information around the code base. My biggest concern is probably the various statuses (their naming and the states they represent).

Also, we need to make sure we have good documentation on the expected behavior of this plugin now.

I agree with this, I think I can change it to be a more uni-directional state-sync where the main-thread pushes the state of the plugin to the workers. The workers then just need to react to the correct state change and run importScripts when necessary.

I think that will let me get rid of the downloaded status as well.

Arindam Bose added 2 commits October 17, 2019 21:48
- main thread now pings workers with an `pluginStateChange` event that syncs the state of the plugin on the main thread with the worker
- worker logic is now reactive to this state change, and will load and parse the plugin on appropriate state change
@arindam1993 arindam1993 changed the title [dnm]Load rtlTextPlugin on-demand using rtlTextPluginURL map option Load rtlTextPlugin on-demand using rtlTextPluginURL map option Oct 18, 2019
@arindam1993 arindam1993 changed the title Load rtlTextPlugin on-demand using rtlTextPluginURL map option Add option to load rtl-text-plugin lazily Oct 18, 2019
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking better. What is the performance impact of deferred loading during a flyTo animation?

Needs a docs/example update once merged (https://github.com/mapbox/mapbox-gl-js-docs/blob/publisher-production/docs/pages/example/mapbox-gl-rtl-text.html)

src/index.js Outdated Show resolved Hide resolved
src/source/rtl_text_plugin.js Outdated Show resolved Hide resolved
src/data/bucket/symbol_bucket.js Outdated Show resolved Hide resolved
Arindam Bose and others added 2 commits October 22, 2019 17:06
Co-Authored-By: Asheem Mamoowala <asheem.mamoowala@mapbox.com>
Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the available status would make more sense if it were deferred instead and I don't think we need to check for webkitURL but otherwise, it looks pretty good

unavailable: 'unavailable',
loading: 'loading',
unavailable: 'unavailable', // Not loaded
available: 'available', // The plugin URL has been specified, but loading has been deferred
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this status should be deferred. i still think available is confusing

return callback;
};

export const clearRTLTextPlugin = function() {
pluginStatus = status.unavailable;
pluginURL = null;
if (pluginBlobURL) {
const URL = window.URL || window.webkitURL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think we can get rid of window.webkitURL unless you know of a specific reason to keep it

@arindam1993 arindam1993 self-assigned this Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants