IonMonkey: Cleanup! Aisle TestCommonPropFunc()!

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: efaust, Assigned: Yaron Tausky)

Tracking

unspecified
mozilla26
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Once much prettier, TestCommonPropFunc() has grown in organic and ugly ways in response to fuzzbugs. A cleanup pass is necessary.
Whiteboard: [ion:t]
(Reporter)

Updated

6 years ago
Depends on: 793284
Eric, are you still working on this?  Could this be a mentored bug instead?
Flags: needinfo?(efaustbmo)
(Reporter)

Comment 2

5 years ago
Could and should. I'd be happy to mentor, as well, if there was someone who wanted to do it.
Assignee: efaustbmo → general
Status: ASSIGNED → NEW
Flags: needinfo?(efaustbmo)
(Reporter)

Updated

5 years ago
Whiteboard: [ion:t] → [ion:t][mentor=efaust][lang=c++]
(Assignee)

Comment 3

5 years ago
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
(Reporter)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
Created attachment 798194 [details] [diff] [review]
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. :)]
(Reporter)

Comment 6

5 years ago
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+
(Assignee)

Comment 7

5 years ago
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.
(Reporter)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
Created attachment 799366 [details] [diff] [review]
793212_refactor_testcommonpropfunc.patch

Seems alright. I changed it according to your comments and fixed the formatting.
Attachment #798194 - Attachment is obsolete: true
Attachment #799366 - Flags: review?(efaustbmo)
(Reporter)

Comment 10

5 years ago
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.
Attachment #799366 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 799812 [details] [diff] [review]
793212_refactor_testcommonpropfunc.patch - revised

Sure, I changed it to a local |bool lcont|. Carrying over the r+.
Attachment #799366 - Attachment is obsolete: true
Attachment #799812 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6c0117fbfe69
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.