Last Comment Bug 899832 - hoist JSScript::originPrincipals and LazyScript::originPrincipals into ScriptSource
: hoist JSScript::originPrincipals and LazyScript::originPrincipals into Script...
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-30 16:43 PDT by Luke Wagner [:luke]
Modified: 2013-08-04 17:28 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
hoist-origin-principals (39.89 KB, patch)
2013-07-30 17:37 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
hoist-origin-principals (45.44 KB, patch)
2013-07-30 19:41 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
hoist-origin-principals (46.59 KB, patch)
2013-08-01 11:17 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2013-07-30 16:43:31 PDT
The "origin principals" are the same for the entire tree of scripts (lazy and eager) created by a single compilation so the hoisting is pretty straightforward.
Comment 1 Luke Wagner [:luke] 2013-07-30 17:37:14 PDT
Created attachment 783465 [details] [diff] [review]
hoist-origin-principals

Double bonus points: the patch gets to remove padding32.

The current originPrincipals "normalization" we do is kinda ad hoc and I would've had to inject it in a random third place so I hoisted the normalization so that it happens at the JSAPI entry points and is asserted in the frontend entry points.
Comment 2 Luke Wagner [:luke] 2013-07-30 19:41:11 PDT
Created attachment 783495 [details] [diff] [review]
hoist-origin-principals

Oops, need to add padding to LazyScript on 32-bit (and a static assert).
Comment 3 Brian Hackett (:bhackett) 2013-08-01 09:30:14 PDT
Comment on attachment 783495 [details] [diff] [review]
hoist-origin-principals

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

Sorry for the delay.  It'd be good to see another version with the JS_DropPrincipals and AssertCompileOptionsNormalized stuff fixed.

::: js/src/jsapi.cpp
@@ +4333,5 @@
>          rt->destroyPrincipals(principals);
>  }
>  
>  JS_PUBLIC_API(void)
> +JS_DropPrincipals(JSPrincipals *principals)

Can you remove this method?  This is not threadsafe and the existing signature for this is better, since threads generally cannot get their hands on a JSRuntime* without going through a threadsafe assert, at least after bug 898886.

::: js/src/jsscript.cpp
@@ +1351,5 @@
>      adjustDataSize(0);
>      js_free(filename_);
>      js_free(sourceMap_);
> +    if (originPrincipals_)
> +        JS_DropPrincipals(originPrincipals_);

To get the JSRuntime* here I think it would be best to supply it as a parameter to ScriptSource::{destroy,decref}, similarly to what is done with IonCode::decref.

::: js/src/jsscript.h
@@ +317,5 @@
>          ready_(true)
>      {
>          data.source = NULL;
> +        if (originPrincipals_)
> +            JS_HoldPrincipals(originPrincipals_);

A main thread assertion on the runtime might be nice here.

@@ +1490,5 @@
> + * JSAPI clients are to leave CompileOptions.originPrincipals in which case the
> + * JS engine makes options.originPrincipals = origin.principals. This
> + * normalization step must before the originPrincipals get stored in the
> + * JSScript/ScriptSource.
> + */

Mangled comment.

@@ +1506,5 @@
> +                                                          options->originPrincipals);
> +}
> +
> +static inline void
> +AssertCompileOptionsNormalized(const CompileOptions &options)

I think this method and NormalizeCompileOptions are really only necessary because principals and originPrincipals are public members of CompileOptions.  And yet CompileOptions has setters for these members!  That struct is pretty incoherent.

Can you make CompileOptions::{principals,originPrincipals} into private members and access them through getters?  Since there is already a setter API clients shouldn't be bothered much by this change.  Then the getter for originPrincipals can just call NormalizeOriginPrincipals.

::: js/src/vm/Debugger.cpp
@@ +4120,5 @@
>       * static level will suffice.
>       */
>      CompileOptions options(cx);
>      options.setPrincipals(env->compartment()->principals)
> +           .setOriginPrincipals(env->compartment()->principals)

I don't think this is necessary with the CompileOptions changes above.

::: js/src/vm/Runtime.cpp
@@ +99,5 @@
> +void
> +PerThreadData::destroyPrincipals(JSPrincipals *principals)
> +{
> +    runtime_->destroyPrincipals(principals);
> +}

This should be removed.
Comment 4 Luke Wagner [:luke] 2013-08-01 09:50:46 PDT
(In reply to Brian Hackett (:bhackett) from comment #3)

> Sorry for the delay.  It'd be good to see another version with the
> JS_DropPrincipals and AssertCompileOptionsNormalized stuff fixed.

Great points, will fix!

> > +        if (originPrincipals_)
> > +            JS_HoldPrincipals(originPrincipals_);
> 
> A main thread assertion on the runtime might be nice here.

JS_HoldPrincipals is threadsafe (it's just a JS_ATOMIC_INCREMENT) so while the assertion would hold, I don't think it'd really be ensuring anything necessary.
Comment 5 Luke Wagner [:luke] 2013-08-01 11:17:47 PDT
Created attachment 784483 [details] [diff] [review]
hoist-origin-principals
Comment 7 Ed Morley [:emorley] 2013-08-02 05:40:30 PDT
https://hg.mozilla.org/mozilla-central/rev/e827cc07b006

Note You need to log in before you can comment on or make changes to this bug.