Open Bug 1224722 Opened 4 years ago Updated Last year

Enable executing multiple embedding-provided inputs as self-hosted JS during runtime startup

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

ASSIGNED

People

(Reporter: till, Assigned: till, NeedInfo)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

This enables compiling multiple buffers or source files as self-hosted JS during startup. The shell has a new --self-hosted option that can be used multiple times to load multiple files as self-hosted JS. More importantly, an embedding can call EvaluateSelfHosted, providing either a buffer or a file name, to feed in self-hosted scripts.

Once the first content global is created using JS_NewGlobalObject, calling EvaluateSelfHosted asserts.
Attachment #8687411 - Flags: review?(efaustbmo)
Attachment #8687411 - Flags: feedback?(mwu)
Attachment #8687411 - Flags: feedback?(bzbarsky)
Comment on attachment 8687411 [details] [diff] [review]
Enable executing multiple embedding-provided inputs as self-hosted JS during runtime startup

>+EvaluateSelfHosted(JSContext* cx, const char* bytes, size_t length, const char* filename);

Is this really bytes?  Or UTF-8 chars?  Or something else?  Put another way, is there a reason it's not char16_t*?  In practice that may not be an issue since I expect all our self-hosted stuff to be ASCII, but...

Apart from that, looks decent.  Though perhaps the API should take a JSRuntime, not JSContext.  It's weird to be passing in a JSContext in the null compartment, to me, and this is conceptually part of runtime init anyway.

The documentation could also use some description of how this interacts with multiple runtimes.  That is, if I create a new runtime, can I always eval self-hosted code on it even if I have already created a global in a different runtime?  If no, when can't I?
Attachment #8687411 - Flags: feedback?(bzbarsky) → feedback+
Thanks for the feedback. The attached version slightly expands on the EvaluateSelfHosted JSAPI function comments, introduces an additional EvaluateSelfHosted overload taking a char16_t buffer, fixes a few bugs and adds a jsapi test that would've caught said bugs.

> Is this really bytes?  Or UTF-8 chars?  Or something else?  Put another way, is there a reason it's not char16_t*?  In practice that may not be an issue since I expect all our self-hosted stuff to be ASCII, but...

The reason was that the three types of input I had (embedded self-hosted code, and code loaded from a file) are UTF8. Really, they're ASCII, as you expected. I added a char16_t* overload and implemented the other two in terms of that one.
Attachment #8687914 - Flags: review?(efaustbmo)
Attachment #8687914 - Flags: feedback?(mwu)
Attachment #8687914 - Flags: feedback?(bzbarsky)
Attachment #8687411 - Attachment is obsolete: true
Attachment #8687411 - Flags: review?(efaustbmo)
Attachment #8687411 - Flags: feedback?(mwu)
Comment on attachment 8687914 [details] [diff] [review]
Enable executing multiple inputs as self-hosted JS during runtime startup. v2

>+ * Evaluate the given file buffer as self-hosted JS.

Should probably say that the expected encoding of the file is UTF-8.
Attachment #8687914 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky [:bz] from comment #4)
> >+ * Evaluate the given file buffer as self-hosted JS.
> 
> Should probably say that the expected encoding of the file is UTF-8.

I changed the comment to say "[l]oad and evaluate the given file path as UTF8-encoded self-hosted JS."
API makes sense, though I can't imagine using the variant that reads the file for you.

(In reply to Till Schneidereit [:till] from comment #3)
> I added a char16_t* overload and implemented the other two in
> terms of that one.

Wouldn't it make more sense the other way around, since things are most likely going to be in ASCII?
(In reply to Michael Wu [:mwu] from comment #6)
> API makes sense, though I can't imagine using the variant that reads the
> file for you.

I guess an embedding wouldn't ever use that in production, no. It's useful for shell testing using --self-hosted=filename, and I re-implemented MOZ_SELFHOSTEDJS in terms of it, though.
> 
> (In reply to Till Schneidereit [:till] from comment #3)
> > I added a char16_t* overload and implemented the other two in
> > terms of that one.
> 
> Wouldn't it make more sense the other way around, since things are most
> likely going to be in ASCII?

The parser consumes char16_t, so we have to feed that into the engine. Should that change at some point, we should definitely add a real ASCII overload.
Attachment #8687914 - Flags: feedback?(mwu) → feedback+
Blocks: 1226551
Comment on attachment 8687914 [details] [diff] [review]
Enable executing multiple inputs as self-hosted JS during runtime startup. v2

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

Nice.

::: js/src/builtin/TestingFunctions.cpp
@@ +3082,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (args.length() != 1 || !args[0].isString()) {
> +        JS_ReportError(cx, "First argument should be a string");

"First and only argument should be a string"?

I don't really care, though, given that it's a testing function.
Attachment #8687914 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ad7b5452d4725cf152e2660e68c3f0bc243e4e
Bug 1224722 - Enable executing multiple inputs as self-hosted JS during runtime startup. r=efaust, f=bz,mwu
https://hg.mozilla.org/integration/mozilla-inbound/rev/f477eb89443b7daffb2a0bbda72fbfa2977b7f32
Bug 1224722 - Enable executing multiple inputs as self-hosted JS during runtime startup. r=efaust, f=bz,mwu
Backed out for Linux x64 opt Valgrind bustage together with bug 1226551: https://hg.mozilla.org/integration/mozilla-inbound/rev/496bd6468e61

https://treeherder.mozilla.org/logviewer.html#?job_id=17903617&repo=mozilla-inbound

==12218== LEAK SUMMARY:
==12218==    definitely lost: 0 bytes in 0 blocks
==12218==    indirectly lost: 0 bytes in 0 blocks
==12218==      possibly lost: 10,181 bytes in 188 blocks
==12218==    still reachable: 168,376 bytes in 537 blocks
==12218==         suppressed: 48,176 bytes in 43 blocks
==12218== Reachable blocks (those to which a pointer was found) are not shown.
==12218== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==12218==
==12218== ERROR SUMMARY: 42 errors from 42 contexts (suppressed: 7 from 7)
Flags: needinfo?(till)
Blocks: 1230640
Comment on attachment 8687914 [details] [diff] [review]
Enable executing multiple inputs as self-hosted JS during runtime startup. v2

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

::: js/src/vm/Runtime.cpp
@@ +357,5 @@
>  
> +bool
> +JSRuntime::completeInitialization(JSContext* cx)
> +{
> +    MOZ_ASSERT(!hasContentGlobals);

I don't think hasContentGlobals is initialized when this assertion is checked.
Oh, looks like your last checkin had that fixed.
You need to log in before you can comment on or make changes to this bug.