[webkit-reviews] review granted: [Bug 135145] FTL should be able to do polymorphic call inlining : [Attachment 237099] the patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 135145: FTL should be able to do polymorphic call inlining
https://bugs.webkit.org/show_bug.cgi?id=135145

Attachment 237099: the patch
https://bugs.webkit.org/attachment.cgi?id=237099&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237099&action=review


> Source/JavaScriptCore/bytecode/CallEdgeProfile.cpp:55
> +	   for (CallEdge entry : m_otherCallees->m_processed) {

CallEdge&?

> Source/JavaScriptCore/bytecode/CallEdgeProfile.cpp:82
> +    for (CallEdge edge : callEdges())

CallEdge&?

> Source/JavaScriptCore/bytecode/CallEdgeProfile.cpp:212
> +	   for (auto entry : *m_otherCallees->m_temporarySpectrum)

I think auto& is best here. The pointed-to items are not super huge, but we
have scripts that flag uses of "for (auto " because it has been a problem in
other places, and it's nice to stay out of the noise.

> Source/JavaScriptCore/bytecode/CallVariant.h:172
> +    JSCell* m_callee;

It looks like this value is ultimately a member of a heap data structure. Does
it need to be a WriteBarrier<JSCell>?

> Source/WTF/wtf/OwnPtr.h:81
> +	   // Construct an object to store into this OwnPtr, but only so long
as this OwnPtr
> +	   // doesn't already point to an object. This will ensure that after
you call this,
> +	   // the OwnPtr will point to an instance of T. This instance may or
may not have been
> +	   // created by this call. Moreover, this call uses an opportunistic
transaction, in
> +	   // that we may create an instance of T and then immediately throw it
away.

Comment should probably mention concurrency: "will point to an instance of T,
even in called concurrently".

> Source/WTF/wtf/Spectrum.h:55
> +	   for (auto entry : otherSpectrum)

auto&