1554362 - Compile non-worker scripts directly from UTF-8 where it makes sense, behind a pref

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task)

Description

[Jeff Walden [:Waldo]]

Sibling to bug 1553502 which was for workers -- this covers everything else on the web. (Or at least most everything else, my grasp of modern spec terminology here is fairly new so I could be missing a scripting possibility.)

Workers were easy, because workers are always UTF-8. (Albeit possibly with validation errors, which get smoothed into U+FFFD REPLACEMENT CHARACTER by the time the JS engine sees script text.)

Everything else, however, could be many possible things. As our code in ScriptLoader::GetScriptSource has it, the text could be:

• Latin-1 for DOM <script> with inline text that is all single-byte
• UCS-2 for DOM <script> with inline text that isn't single-byte (not sure if the text the DOM has on hand can contain lone surrogates? or do those get massaged into U+FFFD earlier in time)
• UTF-16 for non-inline DOM <script> (so encoded because ScriptLoadHandler decodes downloaded content incrementally to UTF-16 -- a sensible choice when JSAPI only natively supports UTF-16 without inflating, but in principle it could have instead decoded to UTF-8 if callers were appropriately adapted)

The current GetScriptSource just does UTF-16 for everything, which means it can effectively return a SourceText<char16_t>.

mozilla::Maybe<JS::SourceText<char16_t>> GetScriptSource(
    JSContext* aCx, ScriptLoadRequest* aRequest);

It could be changed to do UTF-8 for everything, to effectuate this change. This would be simple. But it would not friendly to hiding this behavior a preference, as risk minimization for the short run.

Or we could template it, like

template <typename Unit>
mozilla::Maybe<JS::SourceText<Unit>> GetScriptSource(
    JSContext* aCx, ScriptLoadRequest* aRequest);

and then the pref could control which parametrization we call. Subsequent code would then handle both UTF-8 and UTF-16 possibilities.

But maybe UTF-8 isn't the best choice for all the possible script text sources mentioned above. And perhaps ideally we would not even bother creating a fresh copy of script text (as GetScriptSource does now) if we're going to immediately evaluate the script. (There are other times where we would do an off-thread compilation, and in such cases obviously we can't not-copy the script text. Somewhat disturbingly JS::CompileOffThread just requires the passed-in text stay live that long, which is okay as long as there's always a copy as happens now, but would not work for the more-optimal case possibly worth pursuing here.)

So, I'm not certain what all we would want to do.

It feels like if we have inline Latin-1 script, it would be nice to do a fast is-all-ASCII test and if so do UTF-8, for sure. (There's a function in encoding_rs::mem for this; I'm not sure if we have a C++ binding for it yet.) But even the rare case of non-ASCII Latin-1 probably should just be UTF-8 too.

Downloaded script, maybe we could -- pref-controlled? -- accumulate it as either UTF-8 or UTF-16. Then whichever we have on hand, determines what sort of evaluation/compilation we do.

Anyway. There's some design space to flesh out here, and I am not seasoned in this code. (Although I feel like there's not a ton of code to understand, so I probably have a decent grasp of it even with just a handful of days of looking.) bz, thoughts?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Still need to think about some of the deeper details, but a few notes while I am looking at them:

1. I don't see how we can avoid a copy somewhere even in the evaluate-immediately case. We can't just hand a pointer into a textnode into the JS engine, because the script execution may kill that textnode, modify its text, etc. Or am I misunderstanding the don't-copy suggestion?
2. I'm pretty sure that the inline script case can contain unpaired surrogates, if its text is set from JS. We can write a test to make sure, of course.
3. We do have an IsASCII() function in xpcom/string/nsReadableUtils.h that would do an is-all-ASCII test. It's not clear to me that latin1-to-utf8 conversion is necessarily that much slower than latin1-to-UTF16 byte-inflation, though. Except in practice we seem to have the latter SIMD-accelerated and the former not. Also, the Rust convert_latin1_to_utf8 code requires that the dest buffer be 2x the size of the source buffer. Which I guess is no worse than the UTF-16 buffer size we end up with now...

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Testcase for lone surrogates in script text

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

1. We can't just hand a pointer into a textnode into the JS engine, because the script execution may kill that textnode, modify its text, etc. Or am I misunderstanding the don't-copy suggestion?

I believe -- but I certainly would want to verify this (and document it better in JSAPI docs/doc-comments) before charging forward -- that the provided source only need remain around until the ScriptSource for the script to be compiled/evaluated is created. That ScriptSource has arbitrary lifetime and so necessarily makes a (possibly compressed) copy of the source (or uses the source hook, which would also have to be written against this possibility).

So by the time there's a JSScript* to evaluate, we must have a ScriptSource* for it, and we no longer need the original text. And then when the script is evaluated, any access to source characters would access the ScriptSource's copy -- not the stuff originally passed in.

2. I'm pretty sure that the inline script case can contain unpaired surrogates, if its text is set from JS. We can write a test to make sure, of course.

The attached testcase is my stab at it from a few hours ago. Its source is this:

<!DOCTYPE html>
<html>
<body>
<p id="out"></p>
<script>
document.getElementById("out").textContent =
  (function() { /* bad UTF-16: XXXX */ }).toString().indexOf("\uFFFD");
</script>
</body>
</html>

except that the "XXXX" is instead four U+DFFF code points. It displays 28 for me, so the JS engine observes U+FFFD instead of U+DFFF -- and it appears to me those would be in the original script node text, if I followed Searchfox calls correctly.

3. Also, the Rust convert_latin1_to_utf8 code requires that the dest buffer be 2x the size of the source buffer. Which I guess is no worse than the UTF-16 buffer size we end up with now...

The encoding_rs::mem function to test is-ASCII returns the index of the first non-ASCII (so you check all-ASCII by comparing against the provided length), so you'd do better than double to at least that degree. Tho SIMD to just count the number of bytes with high bit set sounds so embarrassingly parallel I'd think a trimmed memory amount would be pretty feasible too...

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Testcase for lone surrogates in script-generated script text

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)

2. I'm pretty sure that the inline script case can contain unpaired surrogates, if its text is set from JS. We can write a test to make sure, of course.

Oh, whoops, misread. This testcase does seem to indicate this is possible.

Which means translating directly to UTF-8 is not feasible, not unless it's WTF-8. And right now the UTF-8 tokenizing code supports only UTF-8, not WTF-8 -- although it could be made to support WTF-8 with a runtime option, for sure.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So I thought about this some. For inline script, I expect the common case is in fact that it's ASCII-only stored as latin-1. We can add telemetry to verify this.

Furthermore, the common case is that it's all a single buffer in memory, so we could in theory handle that without copying. But per spec there are various cases when it could in fact be a bunch of separate buffers that we need to concatenate... So while it would be nice to have a fast path that does not copy, we would also need to maintain a copying path no matter what.

That said, right now we do two copies (in the latin-1 case): into the nsAutoString, and then into the JS_malloc'd buffer. That's kinda silly and maybe we can do better here.

Anyway, my recommendations are as follows:

1. We should have either a templated GetScriptSource or have it return a discriminated union, whatever seems simpler.
2. For external scripts, if they accumulated as UTF-8 you just return that UTF-8.
3. Inline scripts are mostly a distraction for the moment: they tend to be small and the various copy/conversion costs we are looking at here are low. So for now, let's just do whatever is simplest. Most likely that's keeping it UTF-16 as now if the pref is set to UTF-16 and converting to UTF-8 if the pref is set to UTF-8. I would not worry about non-copying and whatnot until after we have removed the pref and the JS engine is ingesting UTF-8 for external scripts consistently. At that point we can remove some complexity and look into adding the non-sharing complexity.
4. We should add telemetry to see how often external scripts are being decoded from UTF-8 anyway, and probably for how often inline scripts are actually latin1, or single-textnode, or both. Along maybe with some length weighting?

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Bug 1554362 - Add a preference to control whether external script data is accumulated as UTF-8 instead of UTF-16 (and if so, compiled as UTF-8 without inflating to UTF-16). r=bzbarsky

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Bug 1554362 - Implement nsJSUtils::CompileModule for UTF-8 as well as UTF-16. r=bzbarsky

Depends on D34820

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Bug 1554362 - Add a UTF-8 overload of nsJSUtils::ExecutionContext::Compile. r=bzbarsky

Depends on D34821

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Bug 1554362 - Replace the two ScriptLoadHandler::EnsureDecoder overloads with one inline function that fast-paths the already-have-one test and an out-of-line function that tries to provide one presuming none exists. r=bzbarsky

Depends on D34822

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Bug 1554362 - Adjust some obsolete or debatably-misplaced comments in ScriptLoadRequest.h. r=bzbarsky

Depends on D34823

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Bug 1554362 - Accumulate external source text as either UTF-8 or UTF-16, in pref-controlled fashion, and then compile the accumulated text using corresponding JSAPI entrypoints without inflating UTF-8 to UTF-16. r=bzbarsky

Depends on D34824

Comment 11

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)

Anyway, my recommendations are as follows:

1. We should have either a templated GetScriptSource or have it return a discriminated union, whatever seems simpler.

Given inline mucks things up, a discriminated union seemed simplest. I used MaybeOneOf as fitting the desires pretty well, but its API is sadly not the same as Maybe or Maybe<Variant<...>>, so it's a little messy. (At some point I want to make MaybeOneOf be a Variant with a hidden internal type to represent the empty state, maybe. This would also solve Maybe<Variant<...>> having a bool for the Maybe and a separate tag for the Variant, which is a bit space-wasteful.)

2. For external scripts, if they accumulated as UTF-8 you just return that UTF-8.

The accumulation decision has to proceed in lockstep with whether DecodeToUTF8 or DecodeToUTF16 is used. I made ScriptLoadRequest::SetTextSource make the determination as to whether to accumulate into char16_t or Utf8Unit, based on a new pref. Then every downstreap use just checks for which sort of vector exists and is being filled.

3. Inline scripts are mostly a distraction for the moment: they tend to be small and the various copy/conversion costs we are looking at here are low. So for now, let's just do whatever is simplest. Most likely that's keeping it UTF-16 as now if the pref is set to UTF-16 and converting to UTF-8 if the pref is set to UTF-8.

I left it exactly as it is now, as UTF-16. Changing inline script handling can be a separate followup bug or patches very cleanly. MaybeSourceText can hold either sort of data, so it's no biggie.

I would not worry about non-copying and whatnot until after we have removed the pref and the JS engine is ingesting UTF-8 for external scripts consistently. At that point we can remove some complexity and look into adding the non-sharing complexity.

Sure.

We should add telemetry...for how often inline scripts are actually latin1, or single-textnode, or both. Along maybe with some length weighting?

Fodder for the followup bug.

We should add telemetry to see how often external scripts are being decoded from UTF-8 anyway

On further looking at how the architecture of all the script loading is set up, I am not certain this is horribly useful. UTF-8 even for scripts shipped that way is a little odd, because it's decode-with-replacement decoding, while the JS engine currently does decode-and-fail-on-error decoding. This could be altered, but that begins to make me leery about implementing multiple varieties of UTF-8 error handling directly in our tokenizing. And it presents certain complexities, like suddenly algorithms that keep raw pointers into source get more complicated because they're pointing at possible garbage code units.

There's also the matter of a leading BOM in a script being a transport-level encoding thing that ought not affect first-line column numbers, but it would if we made the JS engine do this and didn't "shard" encoding handling across script loader and JS both.

Ultimately it feels to me like, given there's always a decode happening, whether telemetry would indicate the external source is UTF-8 or not is not a horribly usable data point.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ultimately it feels to me like, given there's always a decode happening, whether telemetry would indicate the external source is UTF-8 or not is not a horribly usable data point.

I think my feeling was that if the decode is typically from UTF-8, then decoding to UTF-8 is a no-brainer. But if it's typically from Latin1, then decoding that to UTF-16 is possibly faster than to UTF-8, and so it might be worth doing some measurements to see whether it's better to decode to UTF-16 or UTF-8 in that case.

I definitely wasn't suggesting that SpiderMonkey grow a "decode-with-replacement" mode; I am assuming all that is handled on the Gecko side.

Comment 13

[Jeff Walden [:Waldo]]

Isn't Latin-1 just not an addressable encoding on the web? And if you request it you actually get Windows-1252 instead which is not even close to it? So if it actually were advantageous to inflate to UTF-16, it wouldn't help anyway. :-|

Comment 14

[Boris Zbarsky [:bzbarsky]]

Oh, right you are. You do indeed get Windows-1252 decoding if you claim to be "latin1", which means that you have to do something slow anyway, OK.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e72ea77403d
Add a preference to control whether external script data is accumulated as UTF-8 instead of UTF-16 (and if so, compiled as UTF-8 without inflating to UTF-16).  r=bzbarsky

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1e72ea77403d

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c26e7cea7730
Implement nsJSUtils::CompileModule for UTF-8 as well as UTF-16.  r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/0fc25c40772c
Add a UTF-8 overload of nsJSUtils::ExecutionContext::Compile.  r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/343794d84d32
Replace the two ScriptLoadHandler::EnsureDecoder overloads with one inline function that fast-paths the already-have-one test and an out-of-line function that tries to provide one presuming none exists.  r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/6e90aa400acc
Adjust some obsolete or debatably-misplaced comments in ScriptLoadRequest.h.  r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/613d59f386c7
Accumulate external source text as either UTF-8 or UTF-16, in pref-controlled fashion, and then compile the accumulated text using corresponding JSAPI entrypoints without inflating UTF-8 to UTF-16.  r=bzbarsky

Comment 18

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c26e7cea7730
https://hg.mozilla.org/mozilla-central/rev/0fc25c40772c
https://hg.mozilla.org/mozilla-central/rev/343794d84d32
https://hg.mozilla.org/mozilla-central/rev/6e90aa400acc
https://hg.mozilla.org/mozilla-central/rev/613d59f386c7

Comment 19

[Florin Strugariu [:Bebe]]

== Change summary for alert #21509 (as of Mon, 17 Jun 2019 08:55:41 GMT) ==

Improvements:

3% raptor-tp6-sheets-firefox fcp windows10-64-shippable-qr opt 271.17 -> 263.12
3% raptor-tp6-sheets-firefox windows10-64-shippable opt 332.92 -> 323.13
3% raptor-tp6-sheets-firefox fcp windows10-64-shippable opt 281.19 -> 273.08
3% raptor-tp6-sheets-firefox windows10-64-shippable-qr opt 331.11 -> 322.51
3% raptor-tp6-sheets-firefox fcp windows7-32-shippable opt 279.85 -> 272.75
2% raptor-tp6-sheets-firefox windows7-32-shippable opt 330.25 -> 323.06

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=21509

Comment 20

[Jeff Walden [:Waldo]]

(In reply to Florin Strugariu [:Bebe] (needinfo me) from comment #19)

💯💯💯