793212 - IonMonkey: Cleanup! Aisle TestCommonPropFunc()!

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Once much prettier, TestCommonPropFunc() has grown in organic and ugly ways in response to fuzzbugs. A cleanup pass is necessary.

Comment 1

[Nicolas B. Pierron [:nbp] {away}]

Eric, are you still working on this?  Could this be a mentored bug instead?

Comment 2

[Eric Faust [:efaust]]

Could and should. I'd be happy to mentor, as well, if there was someone who wanted to do it.

Comment 3

[Yaron Tausky (please use my Mozilla profile)]

I'd like to take this one. By cleanup I assume you mean it should be broken down into smaller, easier to handle methods, right?

Comment 4

[Eric Faust [:efaust]]

(In reply to Yaron Tausky from comment #3)
> I'd like to take this one. 

Awesome! Glad to see someone working on it.

> By cleanup I assume you mean it should be broken
> down into smaller, easier to handle methods, right?

Yeah, exactly. There are sorta three phases to the thing right now, but I admit it's a spaghettified mess: The checking loop over the TypeSet, the loop from the original object to the found prototype, and the freezing loop over the typeset (and I guess we can toss guard generation in there, too). Probably it will be a good bet to try and break things up somewhere along these lines, though there are some "interactions at a distance" between them.

Hopefully this makes sense? Let me know if you have any questions.

Comment 5

[Yaron Tausky (please use my Mozilla profile)]

Attachment: Rough draft

I made a rough draft for the changes. Is this approximately the right direction?

[The strange formatting is there to make it easier to compare the new code with the current version, I'll change it in a later version. :)]

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 798194 [details] [diff] [review]
Rough draft

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

In general, this is better than I had hoped for. Much cleaner, and certainly on the right track. :)

I'm interested in your thoughts on the |rv| changes. I'm really not sure if I like that better than just flipping the polarity that was already there.

::: js/src/jit/IonBuilder.cpp
@@ +7628,2 @@
>  inline bool
> +TestClassHasHook(Class *clasp, bool isGetter)

This name is unfortunately too general. Perhaps TestClassHasAccessorHook?

@@ +7739,5 @@
>  
> +        if (WalkUpProtoChain(cx, id, isGetter, foundProto, curObj, rv))
> +            return rv;
> +    }
> +    return false;

I find it a little counter intuitive to use false to mean continue and true to mean stop. I guess the reasoning was "Do I return?". Can we at least flip the polarity?

Perhaps we should even move to a model of:

if (!Func(..., continue))
    return false;
if (!continue)
    return true;

Is that better? It's certainly a little more verbose, but it might be clearer? It at least keeps |return false| as meaning "there was an error", which is the style that IonBuilder uses pervasively.

@@ +7743,5 @@
> +    return false;
> +}
> +
> +inline bool
> +WalkUpProtoChain(JSContext *cx, HandleId id, bool isGetter, JSObject *foundProto,

I think, again, this name is too generic to be hanging around in global namespace.

@@ +7869,5 @@
> +    current->add(wrapper);
> +    wrapper = addShapeGuard(wrapper, foundProto->lastProperty(), Bailout_ShapeGuard);
> +
> +    // Pass the guard back so it can be an operand.
> +    if (isGetter) {

This is pre-existing, but can we make this |if (guardOut)|? There's no reason to conflate the getter status and the existence of the outparam.

Comment 7

[Yaron Tausky (please use my Mozilla profile)]

I was wondering about the |rv| semantics myself (I started with a tri-state enum but found it too verbose) and eventually chose this scheme because it left most of the lines intact. I agree that your solution is much more readable; I'll change it to that.

Could you suggest how to name my WalkUpProtoChain? I'm not really sure how to describe what it does succinctly.

Comment 8

[Eric Faust [:efaust]]

Sorry for lapsing on this for a few days, had a few things to catch up on after the holiday.

I think we want to call it |TestCommonAccessorProtoChain|, which is a mouthful, but at least that's roughly what the function does. I am not deeply wedded to this name, though, so other suggestions are welcome.

Comment 9

[Yaron Tausky (please use my Mozilla profile)]

Attachment: 793212_refactor_testcommonpropfunc.patch

Seems alright. I changed it according to your comments and fixed the formatting.

Comment 10

[Eric Faust [:efaust]]

Comment on attachment 799366 [details] [diff] [review]
793212_refactor_testcommonpropfunc.patch

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

See comment below for small change.

r=me with that. Thanks again for looking into this :).

::: js/src/jit/IonBuilder.cpp
@@ +7726,2 @@
>                  return true;
> +            cont = false;

Instead of passing the input reference, can we just keep every layer local to itself?

so:

SearchCommonPropFunc(... , bool &cont)
{
    cont = false;

    ...

    bool continue;
    if (!TestTypeHasOwnProperty(..., continue))
        return false;
    if (!continue)
        return true;

    ...

    cont = true;
    return true;
}

This seems cleaner than having to reset the value every time.

Comment 11

[Yaron Tausky (please use my Mozilla profile)]

Attachment: 793212_refactor_testcommonpropfunc.patch - revised

Sure, I changed it to a local |bool lcont|. Carrying over the r+.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ed Morley [:emorley]]

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