Bug 149796 – Inline cache repatching should be throttled if it happens a lot

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 262405 [details]
the patch

[Filip Pizlo]

Created attachment 262406 [details]
the patch

[Saam Barati]

Comment on attachment 262406 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262406&action=review

Quickly looked through on my phone but I'll review closely later. One comment:

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:95
> +                // allow here is 2^256 - 2, since the slow paths may increment the count to indicate

Do you mean 256 here and not 2^256?

[Filip Pizlo]

Comment on attachment 262406 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262406&action=review

>> Source/JavaScriptCore/bytecode/StructureStubInfo.h:95
>> +                // allow here is 2^256 - 2, since the slow paths may increment the count to indicate
> 
> Do you mean 256 here and not 2^256?

LOL, of course!

[Saam Barati]

Comment on attachment 262406 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262406&action=review

r=me with a couple comments.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3441
> +    m_profilingForTierUp = true;

This isn't used anywhere. Maybe it should be removed?

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:117
> +        WTF::incrementWithSaturation(coolDownCount);

You could max this out at 8 since we're shifting with this number on an 8-bit integer.

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:158
> +    uint8_t coolDownCount;

Nit: I was initially confused by this name and "countdown". I had thought that maybe this was what "countdown" is. Maybe "numberOfCoolDowns" is more descriptive.

> Source/JavaScriptCore/runtime/Options.h:125
> +    v(unsigned, coolDownBase, 20, nullptr) \

Nit: I think a better name for this is something like "initialCoolDownCount"

[Filip Pizlo]

(In reply to comment #5)
> Comment on attachment 262406 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262406&action=review
> 
> r=me with a couple comments.
> 
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:3441
> > +    m_profilingForTierUp = true;
> 
> This isn't used anywhere. Maybe it should be removed?

Removed.

> 
> > Source/JavaScriptCore/bytecode/StructureStubInfo.h:117
> > +        WTF::incrementWithSaturation(coolDownCount);
> 
> You could max this out at 8 since we're shifting with this number on an
> 8-bit integer.

I could, but that would require more code, and just ensuring that it doesn't wrap around above 255 is all that matters.

> 
> > Source/JavaScriptCore/bytecode/StructureStubInfo.h:158
> > +    uint8_t coolDownCount;
> 
> Nit: I was initially confused by this name and "countdown". I had thought
> that maybe this was what "countdown" is. Maybe "numberOfCoolDowns" is more
> descriptive.

OK.

> 
> > Source/JavaScriptCore/runtime/Options.h:125
> > +    v(unsigned, coolDownBase, 20, nullptr) \
> 
> Nit: I think a better name for this is something like "initialCoolDownCount"

OK.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/190561