Closed Bug 847981 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: djvj, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Clean up chain manipulation (obsolete) — Splinter Review
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.
Attachment #721888 - Flags: review?(bhackett1024)
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.
Attachment #721888 - Flags: review?(bhackett1024) → review+
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.
Attached patch Try 2Splinter Review
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.
Attachment #721888 - Attachment is obsolete: true
Attachment #722507 - Flags: review?(bhackett1024)
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?
Attachment #722507 - Flags: review?(bhackett1024) → review+
(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.
Other comments addressed and pushed:
https://hg.mozilla.org/projects/ionmonkey/rev/acd6327286d3
Waiting for tbpl green.
Green?
Yes, thanks for the reminder.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.