847981 - BaselineCompiler: Clean up and normalize IC stub search and removal.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

This is a minor issue that's been bothering me for a bit.

We do searches and removals on IC chains quite often, and anywhere it's not a simple Kind match, we have to use an ad-hoc linked list traversal loop on site.  This is error prone and seriously ugly.

We should generalize these to use a generic function parametrized on the matcher logic.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Clean up chain manipulation

This adds two base classes:  ICStubMatcher and ICStubAction, which expose a pure virtual call operator.  The check-for-stub-in-chain, remove-stubs-in-chain, and do-things-to-stub-in-chain logic in all places in BaselineIC.cpp is replaced with calls to specific functions passing references to subtypes of these classes.

This is partly in preparation for bug 848122 (which will perform some nontrivial chain surgery), and other complexity coming down the line where we will want to manipulate the IC chain.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 721888 [details] [diff] [review]
Clean up chain manipulation

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

This looks fine, but at a high level I think it would be better if the iteration for the stubs matched the styles used in other parts of the code base.  There are two main styles used:

- The most similar to this one is to use an iteration method with an action class, but template the action class rather than use pure virtual methods.  See, for example, StackFrame::forEachUnaliasedActual.  Using templates would be more consistently optimizable by the compiler I suspect.

- The more common and imo better route is to use a custom Iterator class, so instead of manually walking the stub chain you would do 'for (ICStubIterator iter(...); !iter.done(); iter++) {}'  This is clean and would require less code --- in most cases this patch adds more code and makes things less readable, due to the gunk required to declare new classes with temporary state inside them.  I think that for unlinkStubsMatching you would need an iter.unlinkFront, which could work in a similar way to removeFront on hashtable iterators.

Comment 3

[Kannan Vijayan [:djvj]]

Hmm, that 's good feedback.  I like the iterator idea, avoids this messy business of instantiating private classes everywhere and also is more C++-ey.  I think I'll do that instead of this.

I didn't go the template route because for some reason GCC complained when I tried to use function-private classes as template parameters.  Basically it refused to recognize the template function as an appropriate target when the user defined the parameter class inside the function.  That wasn't a problem with derived-classes-ovverriding-abstract-superclass-methods.

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: Try 2

Re-done with iterators.

There isn't a need to have a special |unlinkFront| method - the unlink interface exposed on the iterator can be unified pretty well.

Two iterators are provided:  ICStubConstIterator and ICStubIterator.  The former only traverses, the latter can unlink stubs that it traverses as well.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 722507 [details] [diff] [review]
Try 2

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

::: js/src/ion/BaselineIC.h
@@ +387,5 @@
>  class ICMonitoredStub;
>  class ICMonitoredFallbackStub;
>  class ICUpdatedStub;
>  
> +// Iterator that works on IC chains.

This comment is the same as the one on ICStubIterator, one or the other should describe the difference between the two.

@@ +804,5 @@
> +    }
> +
> +    ICStubIterator beginChain() {
> +        return ICStubIterator(this);
> +    }

The difference between this and beginChainCost is a little strange, could ICStubIterator's constructor be modified so that it also takes the stub to start at, rather than the fallback stub?

@@ +808,5 @@
> +    }
> +
> +    ICStubIterator endChain() {
> +        return ICStubIterator(this, true);
> +    }

This function and endChainCost aren't used anywhere, can they be removed?

Comment 6

[Kannan Vijayan [:djvj]]

(In reply to Brian Hackett (:bhackett) from comment #5)
> The difference between this and beginChainCost is a little strange, could
> ICStubIterator's constructor be modified so that it also takes the stub to
> start at, rather than the fallback stub?

I'd like to keep this design.

ICStubConstIterator is a general class that can be implicitly constructed from any 'ICStub *' pointer.  It does little more than wrap the 'next()' method and NULL check at the end.

ICStubIterator is more constrained - it's strictly intended to traverse "regular" chains hanging off of an ICEntry and terminated with an ICFallbackStub, and it needs a pointer to the fallback stub to enable its 'unlink' method.

To reflect this dichotomy, the ICStubConstIterator has an implicit public constructor from an ICStub pointer, and can be constructed independently by anybody.. and ICStubIterator has a private constructor and can only be constructed by ICFallbackStub due to a friend relationship between them.

Comment 7

[Kannan Vijayan [:djvj]]

Other comments addressed and pushed:
https://hg.mozilla.org/projects/ionmonkey/rev/acd6327286d3
Waiting for tbpl green.

Comment 8

[Paul Wright]

Green?

Comment 9

[Kannan Vijayan [:djvj]]

Yes, thanks for the reminder.