-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Use mapbox-gl-rtl-text to do Arabic text shaping and bidirectional layout (#3708) #3758
Conversation
Thanks for running the numbers @ChrisLoer! I'm more concerned about the increase in load time than the increase in bundle size. 150-200ms additional load time, on top of an already significant (order 1 second) load time, on high-end hardware, seems like a steep price to pay. Can we dive deeper into this aspect of performance and see what our options are for improving it? |
Well I'm not sure if my 2010-era Macbook is high-end, it certainly doesn't do GL as quickly as my iPhone. 😉 But for parsing javascript I suppose it's probably still pretty good. One option I considered is deferring the load of mapbox-icu-js until we encounter a label that needs it (so this would only help load times with maps that didn't display any Arabic labels). The approach I experimented with was to make the mapbox-icu-js require just expose a (large!) base64 encoded string, and then I had the worker threads decode and evaluate the base64. After playing with it a while, I gave it up as too ugly. |
In practice I suspect reducing bundle size will be the most important way to decrease load time, because (1) downloading the bundle is part of overall load time (although it doesn't show up in my numbers), and (2) the load time seems to be driven pretty much entirely by javascript parsing, which I expect is roughly proportional to the size of the bundle (although parsing a big string literal is probably a lot cheaper than parsing the same amount of code). Looking in the Emscripten runtime, there's a fair amount of code included that we don't use (UTF8/UTF32 support, error reporting and debug tracing code, support for running in javascript environments other than a worker thread, etc.), but just eyeballing it in an editor it looks like a few KB worth of code. On the ICU side, I haven't found anything obviously unnecessary being included. I used the LLVM I'm currently experimenting with the |
Using the |
There's another approach we can try: instead of transpiling straight to javascript, we can compile most of the code to bytecode, and then run the bytecode in an interpreter. In theory, this should give us faster load times (because we don't have to parse so much javascript) but slower run times. It's also possible to start interpreted and switch to compiled to try to get the best of both worlds. I tried the simplest version out: distribute our code as bytecode, load it asynchronously, and then run it in the interpreter.
Unfortunately, I didn't see a great improvement in load time: evaluating mapbox-icu-js still took around 150-200ms. We may be up against a lower bound for this technique because we have to include the "emterpreter" javascript code even if we're not including any of our code directly. Or I may need better techniques for profiling in Chrome! Individual calls to |
Fantastic work! Another option for bundling this is to make it an external plugin that people can opt-in to. This is both simpler than lazy loading when encountering Arabic/RTL, and does not add any costs to developers that use other languages. Meanwhile, we could explore slimming down the Emscripten runtime. |
If the performance implications are unacceptable, we might have to do this as a stop-gap, but it feels pretty unsatisfying. It's a hard choice to push on developers, who know less than we do about the performance implications, and in many cases may not know any more than we do about what languages their users expect to read. Hopefully it would still be helpful for motivated developers like @mushon. @mourner, can I enlist you to do a little performance evaluation in your environment, just collecting data using the build in this PR? I'm worried we'll make too many decisions on just my numbers, and I may be mis-reading the timeline, or loading a pathological section of the map (I usually use the city of Mashhad, Iran, FWIW), or have too much other stuff running on my machine... @kkaefer suggested I look at Cheerp, which transpiles C/C++ code to javascript that uses javascript-style memory instead of emulating a stack and heap. The big attraction for us with that approach is that it has a much more lightweight runtime. The downsides I see (just from a quick read of their docs) are: (1) limited support for C++ would be a big constraint on what code we could pull in, and (2) the license is either GPL or commercial. It's not a strict requirement for an initial fix, but I'm definitely looking for a solution that we can build on for all of our text challenges (kerning/ligature support, indic text, language-aware line breaking). On the gl-native side there are platform-specific APIs that can do a lot of that for us, but on the gl-js side I still haven't seen anything that looks more promising than the ICU/Harfbuzz combo. I looked a little more at using Canvas to draw the text and then upload it as a WebGL texture, and noticed one more significant downside to that approach -- no support for line-breaking. |
I spent a couple hours banging my head against getting Cheerp to build ICU. I didn't get very far, but I think I got far enough to give up (in combination with their unfavorable license). The problem that looked too difficult to fix was also very simple: because of its memory model Cheerp can't do memcpy on untyped pointers. The C parts of ICU are filled with code that moves raw bytes around in memory. I could start trying to put types on all of that code, but that seems like a bottomless 🕳️ There is CheerpJ, which we could use to pull in the Java version of ICU, but that's not even released yet, and I'm very skeptical it would end up being faster/lighter. |
I stripped the emscripten runtime as much as I felt I could get away with (I only touched the native javascript part of the runtime -- there's a large compiled part and I'd have to dig into the Emscripten source to try to thin that down). Amazingly, the maps still worked after all my hacking. Here are the updated code size stats:
But the darn thing still takes 150-200 ms to parse. |
Ha! It wasn't really a parsing cost, it's just that at parse time Emscripten creates one giant ArrayBuffer to represent all possible memory. Without understanding what it meant, I picked up a 256 MB "TOTAL_MEMORY" setting from a starter project -- and it was allocating that ArrayBuffer that was taking so long (curiously, I never saw JS Heap usage go over 30 MB in my testing, and I didn't see the process memory usage jump by a large amount either, so I don't know what exactly the browser is doing with that big call). I just tested with a 16 MB TOTAL_MEMORY limit, and the code loads in 40ms (and it seems to be enough memory for ICU to work). I'll have to determine whether there's a safe (relatively low) memory limit I can put on the ICU code, or re-compile with the ALLOW_MEMORY_GROWTH option, which sounds safest but also is supposed to slow things down. It looks like I can shave about 28KB (9KB compressed) off the bundle size by hacking down the run time -- so a 173 KB bundle would be 30% larger than the baseline, instead of the current 182 KB (44% larger). That would be nice to have, but I'm not sure it's worth having to kind-of-fork Emscripten. |
55de7c2
to
ac20a97
Compare
Stats for the latest version of this PR:
|
@ChrisLoer great progress! I'll explore performance on this branch further next week for another perspective. One thing to note is that code evaluation is not the biggest bottleneck — in my previous research on time-to-first-render (#3153), I discovered that worker instantiation time linearly depends on the size of the bundle. So if our current TTFR is around 2s, it is likely to get to ~3s with the ICU build.
I think they know in most cases because it's a part of the style designing process. Our base styles have english transliterations by default. |
To me, that seems like an argument for separating the icu-js code entirely from the main bundle. We could do something like:
That way, you'd have essentially no effect on maps that didn't need the functionality, and for maps that did need it you'd get the (maybe more graceful?) behavior of loading the base map quickly and then filling in labels. Hopefully the labels could fill in pretty quickly, although you'd introduce extra latency by waiting until the worker was already running to start the network request for the ICU code. I don't have a good handle on how bad it is to split up the monolithic mapbox-gl.js bundle, though. We already have the dependency on asynchronously loading data from api.mapbox.com... would this be different hosting-wise because of the size, or because it's "code" instead of data? Thinning the emscripten runtime would still help either way, but it seems like it might not be enough in the "monolithic" case, and it might not be necessary if we're lazy-loading for Arabic labels. |
Let's not do bundle splitting unless we've exhausted all other options. Splitting up the bundle adds significant friction to many use cases:
|
For what it’s worth, the lack of bidi and complex text shaping in Mapbox GL was the reason that Mapbox-designed styles wound up using Chicken, meet egg. Egg, chicken. 🐣 |
I've updated the PR to run the closure compiler against the Emscripten preamble (the "native" javascript side of their runtime). It provides a very modest improvement by stripping out a few totally unused functions: from 803/182KB to 789/181KB. Here are the things I was able to strip manually (~14KB worth) but can't easily remove without forking (or submitting change to) Emscripten:
|
I've had success with overriding |
@kkaefer Thanks for the pointer! I think the big thing there for me to dig into is whether I'm getting any exception-unwinding code pulled into my output even though none of the live code paths should catch or throw exceptions. If TTFR really is directly linearly related to uncompressed bundle size, maybe the most promising approach is to see what we can do to make the memory initializer smaller while keeping it in the bundle. The memory initializer is about 177KB minified, but if we take the binary, compress it, and then base 64 encode it, that's only 16KB. I bet we could find/make a javascript |
I just pushed a commit to stop using Using the
Other than the somewhat extraneous C++ code, the set of compiled function calls looks like it's pretty well limited to what we're actually using for shaping and bidirectional support. I'll turn my attention towards modifying Emscripten -- it looks more promising than modifying ICU, and hopefully we'll be able to take advantage of the same modifications when we try to pull in HarfBuzz. |
I made a kind of hacked-together build that includes It's hard for me to tell the overall effect on performance -- loading the map takes on the order of 2-4 seconds in all of these cases. Lazy-loading the ICU components takes about 200-300ms on each worker, of which roughly 2/3 is the I tried logging
If I squint at those numbers long enough, I see something like a 15% penalty in load time for the two cases where ICU gets used, and not much of a load time penalty for the last case where ICU is included in the bundle but not decompressed/parsed at runtime (for that test I kept rendering the same view of Iran, but just disabled the runtime loading). |
@mourner I just started looking at how some of our other plugins work -- they seem to hook in on the main thread with a call to |
@ChrisLoer we currently have a mechanism in place for custom sources here: https://github.com/mapbox/mapbox-gl-js/blob/master/js/source/worker.js#L83 — it allows you to pass an url which is then evaluated on the worker side. We might try to generalize it for the ICU case, @anandthakker what do you think? |
9e2cfbf
to
0d01480
Compare
The latest commit splits the
I'm very uncertain about the design of the "worker plugins", but at least the basic approach works. I'm getting page load times around 1300ms, but I can't directly compare that to the previous numbers since I finally upgraded to a new Macbook and everything is deliciously faster. With the code split out into a separate plugin, I'm not sure where to put the Arabic tests (maybe build a test harness that lives with |
This plugin isn't really needed for Hebrew anyways, right? Combining diacritics aren't used in OpenStreetMap, and medial/final variants are encoded separately in Unicode. If it is necessary for Hebrew, how about |
The plugin does bidi, which is necessary for Hebrew.
"ar-he" could work, although it seems less immediately apparent what it means.
… On Jan 13, 2017, at 6:17 PM, Minh Nguyễn ***@***.***> wrote:
This plugin isn't really needed for Hebrew anyways, right? Combining diacritics aren't used in OpenStreetMap, and initial/final variants are encoded separately in Unicode.
If it is necessary for Hebrew, how about mapbox-gl-ar-he, using ISO 639 codes for brevity?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
As the non-official Hebrew speaker I am ok with Hebrew not being explicitly mentioned. Especially when Urdu and Farsi aren't mentioned by language names either. I think 'rtl' or 'bidi' can work (even if the script does more than just bidi). Otherwise 'mena' is often used to refer to things pertaining to "Middle East North Africa".
Mushon Zer-Aviv
Mushon.com | Shual.com | @mushon
… On Jan 14, 2017, at 19:49, Chris Loer ***@***.***> wrote:
The plugin does bidi, which is necessary for Hebrew.
"ar-he" could work, although it seems less immediately apparent what it means.
> On Jan 13, 2017, at 6:17 PM, Minh Nguyễn ***@***.***> wrote:
>
> This plugin isn't really needed for Hebrew anyways, right? Combining diacritics aren't used in OpenStreetMap, and initial/final variants are encoded separately in Unicode.
>
> If it is necessary for Hebrew, how about mapbox-gl-ar-he, using ISO 639 codes for brevity?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
8158a30
to
547543b
Compare
Settled on |
- Full support for Unicode Bidirectional Algorithm - Supports shaping Arabic script
547543b
to
0bb4a9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this! 🌏 🌍 🌎
For anyone who's interested in trying these changes out before our February release, you can now clone the |
@ChrisLoer Thank you Chris for this guide to clone the master branch. When would be the official date to release mapbox-gl.js . You mentioned February, but is it like between 1-5th Feb or 25th to 30th Feb..if it is for end of Feb, then I might have to put hands on cloning and testing it sooner. thanks. |
@Kashian We ended up pushing our next release a little earlier than planned, you can play with the changes in |
@ChrisLoer That is fantastic. You always come with good news. I am going to test it against Persian Map tonight (Melbourne time) and will post the results here. I probably will use a custom Persian font for the map. |
@ChrisLoer |
Does your custom Persian font contain glyphs for the Arabic Presentation Forms-B character block? |
@Kashian Great to see RTL is working for you! You're right to note that Studio preview doesn't currently use the RTL plugin: before we either fold the plugin into the main JS bundle or figure out a clear way to make a "toggle" in Studio, we don't want to mislead Studio users by making RTL text appear to work for everyone in preview. Did you make any other changes to your style, so you can make sure it was getting updated correctly? I confess I'm not very knowledgable about that process, but my first guess would be that there's some delay on getting the new style into the map. If you can verify that the style is.... wait @1ec5 just beat me to it. Yeah, if your font doesn't have the presentation forms, then we would fall back to a base font. |
@ChrisLoer Thanks. I just went out and came home and checked it again ! wonderfully this time it works with my TTF font. So obviously problem is resolved now and yes, @1ec5 , The TTF font had the Arab Presentation Forms-B. For the reference, I paste the result back again here. Default original font (used for park name) and the font I replaced for name of roads. Both works fine. Something must have been updated in past few hours. I will check more on shape of words on the corners and sharp turns and even use 2-3 more fonts to see what comes up. Thanks. |
@ChrisLoer first of all, you're a superhero, and you have our full support in keeping up with the good work you've been doing so far and seeing the process through until RTL languages are fully represented by default in every Mapbox map. |
@mushon I like the mockup! 😄 I understand that it's going to be difficult to design an RTL map in Studio when you're "blind" to label appearance, and I totally agree that RTL support isn't done until it shows up through the whole stack. Unfortunately, I can't give you an estimate yet on when we'll have the support into Studio -- we're still figuring things out as we broaden our script support (RTL was just our first and highest priority). Doing something like you suggested in your mockup is definitely on our radar as a possible stop-gap. I can tell you that I won't consider myself done with the script support project until we have full support in Studio as well. |
@ChrisLoer, I understand you don't want the Mapbox Studio to misrepresent what can be done with Mapbox and since RTL is not fully supported in mobile it is not rolled into the studio, but in the meanwhile, we can't really design our maps as we have that exact problem - Mapbox Studio does not represent how our design will actually look like. Is there a way to maybe include this as a Chrome Extension or something, this stop-gap is even more needed now that we actually expect Mapbox to serve RTL languages. (thanks!) |
@mushon @ericrwolfe is working on an iOS app that will do live preview of studio edits -- it's not public yet, but it might be the closest stop-gap we have on the way. |
thank @ChrisLoer, I am not sure how would an iOS app be used in the map style design process. However, a quick blogpost allowing to hack the studio with a chrome extension could push us very quickly forward. Especially on acute mission critical features like running the RTL plugin in Mapbox Studio. |
Fix issue #3708 and issue #3707 by using an Emscripten port of ICU to do Arabic text shaping and bidirectional layout.
The bulk of the functionality is implemented in https://github.com/mapbox/mapbox-icu-js/blob/master/src/icu.js, https://github.com/mapbox/mapbox-icu-js/blob/master/src/ushape_wrapper.c, and https://github.com/mapbox/mapbox-icu-js/blob/master/src/ubidi_wrapper.c.
Adding the ICU/Emscripten dependency adds considerable size to the mapbox-gl.js package and increases page load times. The current mapbox-icu-js build is the smallest I've been able to get by using supported Emscripten and ICU configuration options. I also run
unassert
on the Emscripten runtime to filter out unwanted asserts. It is probably possible to make further modifications within the ICU/Emscripten code to strip out unneeded ICU or runtime functionality.In this PR, all ICU/Emscripten code is pulled directly into the mapbox-gl.js bundle. I also experimented with moving the Emscripten memory initialization data into an asynchronously fetched file in order to decrease JS parse times. Reloading the debug map several times in Chrome on my Macbook, I saw something on the order of 200ms extra page load time, with total scripting time running on the order of 1000ms. I did less testing but saw similar behavior on Firefox and Safari -- in all cases we depend on the browser caching JS compilation results so that the work isn't duplicated across the worker threads. The actual processing of labels once the map is loaded doesn't show up as a noticeable cost.
When/if we add HarfBuzz support for complex text shaping, we will at least get more mileage out of shared Emscripten components like the JS implementation of the C runtime.
cc @mourner @jfirebaugh @lucaswoj @pveugen