1506902 - Change JS::EvaluateUtf8 to directly compile the provided UTF-8 data, without inflating through UTF-16

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jeff Walden [:Waldo]]

We want to incrementally switch the existing UTF-8 compilation APIs to not first inflate to UTF-16.

Most such APIs are used very widely.  Changing these APIs would change vast swaths of web-reachable paths -- probably a bit too quickly/aggressively.

Others are called only in jsshell/xpcshell/ipc/testshell.  Changing these makes shells temporarily less similar to the browser, which possibly could prevent finding browser bugs.

But there's a rough middle option.  JS::EvaluateUtf8 is mostly used by the shells, but it has precisely *one* use in the browser: to evaluate self-hosting code that we control, fixed at build time.

https://searchfox.org/mozilla-central/search?q=symbol:_ZN2JS12EvaluateUtf8EP9JSContextRKNS_22ReadOnlyCompileOptionsEPKcmNS_13MutableHandleINS_5ValueEEE&redirect=false

Changing this equally affects browser and shells, yet more narrowly than making many paths to script execution, of user-controllable data, work differently.

JS::EvaluateUtf8 not inflating is a good, cautious first step toward shipping native UTF-8 tokenizing everywhere.  Let's change it over.  This passes jstests/jit-tests locally; will do a try-run before landing, of course.

(This depends on a mess'o'patches I have locally and are awaiting review places.  The one dep is just the last certain bug I know about in UTF-8 -- you gotta follow the dependency tree back to get to all the other dependencies.)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

JS::EvaluateUtf8Path can't be changed right now because it's used by the shell's LoadScript function, which is used to implement load(), and there's *one* test of asm.js stuff that checks that asm.js compilation happened, but right now asm.js is only supported for UTF-16 compilation.  No reason we couldn't extend asm.js to UTF-8 as well.  But how much effort to we want to sink into a dead-end technology for this, and at what cost in binary size?  Questions for another bug, that maybe I'll file after this one.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 9024790 [details] [diff] [review]
Patch

Review of attachment 9024790 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! One thing I noticed is that we have a gdb-test that looks like it should hit the asm.js issue:

https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/js/src/gdb/tests/test-asmjs.cpp#34

Maybe just change that test to inflate for now? :/ r=me on a trivial test fix because that's not the most important code.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Waldo, could you have a look please?

Comment 4

[Jeff Walden [:Waldo]]

This is deliberate; there are still issues blocking this landing, namely bug 1504947. (Maybe the bot-tool could not nag on bugs that are depending on other opened bugs?)

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Waldo, could you have a look please?

Comment 6

[Jeff Walden [:Waldo]]

Again, comment 4.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fe7603bf2fc
Minor syntactic adjustments to gdb/tests/test-asmjs.cpp, extracted from broader changes related to UTF-8 parsing.  r=jandem

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/5fe7603bf2fc

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1664505ad313
Change JS::EvaluateUtf8 to directly compile the provided UTF-8 data, without inflating through UTF-16.  r=jandem

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/1664505ad313