801839 - IonMonkey: Clean up jsop_getprop().

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: patch

I've been complaining about jsop_getprop() for a while. This finally cleans it up.

Comment 1

[Nicolas B. Pierron [:nbp]]

Comment on attachment 671587 [details] [diff] [review]
patch

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

Nice refactoring moving complex if statements in well named functions which can easily be commented or sorted differently.  The emitted pattern is a bit difficult to read at first but is convenient because we can reuse the abort function for the return value of any sub-functions.

The only trouble that need to be addressed during reviews is that no “return true;” is added after any MIR allocation or graph modification.

Most of our optimizations phase can be rewritten with this “emitted pattern” instead of using a 3 state enum return, such as inlining of native functions.

::: js/src/ion/IonBuilder.cpp
@@ +5801,5 @@
> +
> +    // Try to hardcode known constants.
> +    if (!GetPropTryConstant(&emitted, obj, id) || emitted)
> +        return emitted;
> +    

nit: trailing white space

@@ +5833,5 @@
> +    return pushTypeBarrier(call, types, barrier);
> +}
> +
> +bool
> +IonBuilder::GetPropTryArgumentsLength(bool *emitted)

nit: follow the convention used for IonBuilder.  The first upper case letter is used for static functions.

@@ +5835,5 @@
> +
> +bool
> +IonBuilder::GetPropTryArgumentsLength(bool *emitted)
> +{
> +    *emitted = false;

nit: emitted should always be false, no need to reset it.  Otherwise assert that it is still false.

@@ +5843,5 @@
>          return abort("Type is not definitely lazy arguments.");
> +    if (isArguments != DefinitelyArguments)
> +        return true;
> +
> +    if (JSOp(*pc) == JSOP_LENGTH) {

nit: Invert the condition and remove the braces.

@@ +5903,4 @@
>  
> +    TypeOracle::Unary unary = oracle->unaryOp(script_, pc);
> +    TypeOracle::UnaryTypes unaryTypes = oracle->unaryTypes(script_, pc);
> +    types::StackTypeSet *barrier = oracle->propertyReadBarrier(script_, pc);

Make sure that TI is not freezing twice.

@@ +5946,2 @@
>      if (!TestCommonPropFunc(cx, unaryTypes.inTypes, id, &commonGetter, true, &isDOM))
> +        return false;

This TestCommonPropFunc if-statement sounds weird or badly named, it sounds like the commonGetter flag behaves in a similar way as the emitted flag.  Can you re-use the same pattern for this function.

@@ +6036,5 @@
> +    // The input value must either be an object, or we should have strong suspicions
> +    // that it can be safely unboxed to an object.
> +    if (unary.ival != MIRType_Object && !unaryTypes.inTypes->objectOrSentinel())
> +        return true;
> +        

nit: trailing white-space.

@@ +6073,3 @@
>  
> +    if (oracle->propertyReadAccessGetter(script_, pc))
> +        monitorResult(load, barrier, types);

nit: Reuse the variable “accessGetter” in the condition.

Comment 2

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/73f735acc3e1

Comment 3

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/55d64487e54a

Backed out: M1 failures.

Comment 4

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/896e8053f724

Comment 5

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/896e8053f724