Closed Bug 934419 Opened 10 years ago Closed 10 years ago

Split CompartmentOptions off from ContextOptions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

(Whiteboard: [Shumway:m2][qa-])

Attachments

(2 files, 3 obsolete files)

      No description provided.
Blocks: 885526
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Shumway:m2]
Attachment #827293 - Flags: review?(bobbyholley+bmo)
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?
Attachment #827293 - Flags: review?(bobbyholley+bmo) → review-
(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').
(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.
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.
Attachment #827293 - Attachment is obsolete: true
Attachment #829175 - Flags: review?(bobbyholley+bmo)
This patch adds the override mechanism we discussed on irc.
Attachment #829177 - Flags: review?(bobbyholley+bmo)
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.
Attachment #829175 - Attachment is obsolete: true
Attachment #829175 - Flags: review?(bobbyholley+bmo)
Attachment #829222 - Flags: review?(bobbyholley+bmo)
Attachment #829177 - Attachment is obsolete: true
Attachment #829177 - Flags: review?(bobbyholley+bmo)
Attachment #829225 - Flags: review?(bobbyholley+bmo)
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.
Attachment #829222 - Flags: review?(bobbyholley+bmo) → review+
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);
Attachment #829225 - Flags: review?(bobbyholley+bmo) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/6a9e00928bb8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 938907
No longer depends on: 938907
Depends on: 940305
No longer depends on: 940305
Whiteboard: [Shumway:m2] → [Shumway:m2][qa-]
You need to log in before you can comment on or make changes to this bug.