Closed Bug 793212 Opened 7 years ago Closed 6 years ago

IonMonkey: Cleanup! Aisle TestCommonPropFunc()!


(Core :: JavaScript Engine, defect)

Not set





(Reporter: efaust, Assigned: yaron.tausky)



(Whiteboard: [ion:t][mentor=efaust][lang=c++])


(1 file, 2 obsolete files)

Once much prettier, TestCommonPropFunc() has grown in organic and ugly ways in response to fuzzbugs. A cleanup pass is necessary.
Depends on: 793284
Eric, are you still working on this?  Could this be a mentored bug instead?
Flags: needinfo?(efaustbmo)
Could and should. I'd be happy to mentor, as well, if there was someone who wanted to do it.
Assignee: efaustbmo → general
Flags: needinfo?(efaustbmo)
Whiteboard: [ion:t] → [ion:t][mentor=efaust][lang=c++]
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?
Assignee: general → yaron.tausky
(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.
Attached patch Rough draft (obsolete) — Splinter Review
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 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.
Attachment #798194 - Flags: feedback+
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.
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.
Seems alright. I changed it according to your comments and fixed the formatting.
Attachment #798194 - Attachment is obsolete: true
Attachment #799366 - Flags: review?(efaustbmo)
Comment on attachment 799366 [details] [diff] [review]

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?


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.
Attachment #799366 - Flags: review?(efaustbmo) → review+
Sure, I changed it to a local |bool lcont|. Carrying over the r+.
Attachment #799366 - Attachment is obsolete: true
Attachment #799812 - Flags: review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.