934419 - Split CompartmentOptions off from ContextOptions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Allow JIT flags to be overridden on a per-compartment basis

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 827293 [details] [diff] [review]
Allow JIT flags to be overridden on a per-compartment basis

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

::: js/src/jsapi.cpp
@@ +974,5 @@
> +{
> +    CompartmentOptions &options = CompartmentOptionsRef(context_->compartment());
> +    if (options.baseline() != CompartmentOptions::OPTION_DEFAULT)
> +        return options.baseline() == CompartmentOptions::OPTION_TRUE;
> +    return baseline_;

This is pretty awkward. I think we should add some smarts to our ternary type, and then do:

return context->compartment()->options()->toBoolean(baseline_);

where the parameter of toBoolean() is the default value for the undefined case.

::: js/src/jsapi.h
@@ +1620,5 @@
>          return *this;
>      }
>  
>    private:
> +    JSContext *context_;

Given that there's no accessor, I don't think we need the underscore.

@@ +2586,5 @@
> +    enum Option {
> +        OPTION_DEFAULT,
> +        OPTION_FALSE,
> +        OPTION_TRUE
> +    };

This is no longer the preferred naming convention for enum values in spidermonkey. See the style guide (I think).

Maybe we should call this 'Ternary'? Your call, I just thought I'd suggest it. You might want to ask a spidermonk.

Also, maybe it should be a general utility type and not an inner class?

Comment 3

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #2)
> Comment on attachment 827293 [details] [diff] [review]
> Allow JIT flags to be overridden on a per-compartment basis
> 
> Review of attachment 827293 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsapi.cpp
> @@ +974,5 @@
> > +{
> > +    CompartmentOptions &options = CompartmentOptionsRef(context_->compartment());
> > +    if (options.baseline() != CompartmentOptions::OPTION_DEFAULT)
> > +        return options.baseline() == CompartmentOptions::OPTION_TRUE;
> > +    return baseline_;
> 
> This is pretty awkward. I think we should add some smarts to our ternary
> type, and then do:
> 
> return context->compartment()->options()->toBoolean(baseline_);
> 
> where the parameter of toBoolean() is the default value for the undefined
> case.

My idea was this: once the default values are moved from JSContext to JSRuntime, these particular functions can be removed. On JSCompartment, the accessors for these options can then simply return a bool. The override mechanism will be hidden behind these accessors. The setter function for these options will be split into two function: set<Option>(bool flag) and reset<Option>() (where <Option> is the name of the option).

This way, the ternary type won't ever be used outside the JSCompartment class, so it can be a private nested class/enum/whatever. Does that sound like a reasonable approach to you?

We can definitely do what you suggested too and make the ternary type a bit smarter than a simple enum, but since it will only be used by these four accessors for the JIT options, that feels a bit like overkill. Do you feel strongly about this?

> 
> ::: js/src/jsapi.h
> @@ +1620,5 @@
> >          return *this;
> >      }
> >  
> >    private:
> > +    JSContext *context_;
> 
> Given that there's no accessor, I don't think we need the underscore.
> 

And once somebody needs an accessor for it, he/she will have to find/replace all references to context with context_. This happened to me several times already and it's quite annoying. With that in mind, I think it's good style to clearly distinguish member from non member variables, even when there is no accessor for them, so unless there is a SpiderMonkey style rule that explicitly forbids this I'm inclined to disagree here.

> @@ +2586,5 @@
> > +    enum Option {
> > +        OPTION_DEFAULT,
> > +        OPTION_FALSE,
> > +        OPTION_TRUE
> > +    };
> 
> This is no longer the preferred naming convention for enum values in
> spidermonkey. See the style guide (I think).

Agreed.

> 
> Maybe we should call this 'Ternary'? Your call, I just thought I'd suggest
> it. You might want to ask a spidermonk.
> 
> Also, maybe it should be a general utility type and not an inner class?

I asked Waldo, and he doesn't think it should be a general utility type.

About the name: I feel that Ternary doesn't really convey the semantics of the type. The fact that it's 'ternary' is an implicit property of the type, implying that it can only have three values. This is already apparent from the enum itself. Whenever I see an enum like this, I'm more interested in what its values represent, than how many possible values it can have (to illustrate my point, I think 'boolean' similarly is a better name than 'binary').

Comment 4

[Bobby Holley (:bholley)]

(In reply to Eddy Bruel [:ejpbruel] from comment #3)
> This way, the ternary type won't ever be used outside the JSCompartment
> class, so it can be a private nested class/enum/whatever. Does that sound
> like a reasonable approach to you?

Yes.

> We can definitely do what you suggested too and make the ternary type a bit
> smarter than a simple enum, but since it will only be used by these four
> accessors for the JIT options, that feels a bit like overkill. Do you feel
> strongly about this?

I just think it's not very much work to wrap a struct around the enum and give it a toBoolean method, and it makes the implementation of all of the Compartment option accessors less error prone (there are only 4 now, but that might change). I don't feel all that strongly, but I would need to be convinced that there's some compelling reason not to do it this way.

> And once somebody needs an accessor for it, he/she will have to find/replace
> all references to context with context_. This happened to me several times
> already and it's quite annoying. With that in mind, I think it's good style
> to clearly distinguish member from non member variables, even when there is
> no accessor for them, so unless there is a SpiderMonkey style rule that
> explicitly forbids this I'm inclined to disagree here.

That's a fair point, but we should probably clarify this one way or another in the style guide. Can you bring it up with jorendorff?


> > Also, maybe it should be a general utility type and not an inner class?
> 
> I asked Waldo, and he doesn't think it should be a general utility type.
> 
> About the name: I feel that Ternary doesn't really convey the semantics of
> the type. The fact that it's 'ternary' is an implicit property of the type,
> implying that it can only have three values. This is already apparent from
> the enum itself. Whenever I see an enum like this, I'm more interested in
> what its values represent, than how many possible values it can have (to
> illustrate my point, I think 'boolean' similarly is a better name than
> 'binary').

SGTM.

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: Route all JIT option flags via the active compartment

This patch routes all JIT option flags via the active compartment where appropriate. It does not provide any mechanism yet to override flags on a per compartment basis. I will do that in separate patch.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Allow JIT option flags to be overriden on a per compartment basis

This patch adds the override mechanism we discussed on irc.

Comment 7

[Eddy Bruel [:ejpbruel]]

Turns out I didn't get it quite right. TypeZone::init should copy over the default typeInference flag from the context, not the potentially overridden flag from the compartment.

To test whether type inference is enabled, we call JSContext::typeInferenceEnabled, which checks if the option is set for the zone of the active compartment.

In other words, zones copy over the default setting of the flag from the context, and compartments should override this default on the former, not the latter.

I've will update the patches so that TypeZone::init keeps copying over the default flag from the context, and CompartmentOptions::typeInference forwards to the default flag on its zone, rather than the context.

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: Route all JIT flags via the active compartment

Comment 9

[Eddy Bruel [:ejpbruel]]

Attachment: Allow JIT flags to be overridden on the compartment

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 829222 [details] [diff] [review]
Route all JIT flags via the active compartment

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

::: js/src/jsapi.cpp
@@ +2563,5 @@
> +
> +bool
> +JS::CompartmentOptions::typeInference(const ExclusiveContext *cx) const
> +{
> +    return cx->compartment()->zone()->types.inferenceEnabled;

So, is there a reason that TI needs to be per-zone, or is it just a historical thing? Either way, please document the reasoning here in a comment.

@@ +2599,5 @@
> +    return compartment->options();
> +}
> +
> +JS::CompartmentOptions &
> +JS::ContextCompartmentOptionsRef(JSContext *cx)

I'd just call this CompartmentOptionsRef and let the overload resolution sort it out.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 829225 [details] [diff] [review]
Allow JIT flags to be overridden on the compartment

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

::: js/src/jsapi.cpp
@@ +2563,5 @@
>  
>  bool
>  JS::CompartmentOptions::typeInference(const ExclusiveContext *cx) const
>  {
> +    return typeInferenceOverride_.get(cx->compartment()->zone()->types.inferenceEnabled);

Document the per-zone-ness here as well.

::: js/src/jsapi.h
@@ +2585,5 @@
> +      public:
> +        Override() : value_(Default) {}
> +
> +        bool get(bool value) const {
> +            return value_ == Default ? value : value_ == True ? true : false;

ugh, nested ternary operator. Please make this a non-inline control structure (switch or ifs), or at the very least use parens.

@@ +2604,5 @@
> +            False
> +        };
> +
> +        Value value_;
> +    };

The naming here needs to be reworked. "Value" here shadows JS::Value, and the distinction between |value_| (of type Value) and the argument |value| (of type bool) is super confusing.

How about:
|enum Ternary|
Ternary _value;
bool get(bool defaultVal);
void set(bool arg);

Comment 12

[Eddy Bruel [:ejpbruel]]

(In reply to Bobby Holley (:bholley) from comment #11)
> 
> @@ +2604,5 @@
> > +            False
> > +        };
> > +
> > +        Value value_;
> > +    };
> 
> The naming here needs to be reworked. "Value" here shadows JS::Value, and
> the distinction between |value_| (of type Value) and the argument |value|
> (of type bool) is super confusing.
> 
> How about:
> |enum Ternary|
> Ternary _value;
> bool get(bool defaultVal);
> void set(bool arg);

I agree. However, I tried using:

enum Ternary { DefaultValue, True, False }

only to find out that Linux really doesn't like True and False as enum names:
https://tbpl.mozilla.org/?tree=Try&rev=3d0b26d6d002

To work around this, I settled on the following instead:

enum Mode { Default, ForceTrue, ForceFalse }

Other than that, I've followed your suggestions.

Comment 13

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a9e00928bb8

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6a9e00928bb8