Bug 149796

Summary: Inline cache repatching should be throttled if it happens a lot
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, cdumez, cmarcelo, commit-queue, ggaren, mark.lam, mhahnenb, msaboff, nrotem, oliver, rniwa, saam, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
the patch
none
the patch saam: review+

Description Filip Pizlo 2015-10-04 12:11:53 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2015-10-04 12:24:52 PDT
Created attachment 262405 [details]
the patch
Comment 2 Filip Pizlo 2015-10-04 12:26:42 PDT
Created attachment 262406 [details]
the patch
Comment 3 Saam Barati 2015-10-04 12:57:25 PDT
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?
Comment 4 Filip Pizlo 2015-10-04 13:28:13 PDT
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!
Comment 5 Saam Barati 2015-10-04 23:10:49 PDT
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"
Comment 6 Filip Pizlo 2015-10-05 10:05:53 PDT
(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.
Comment 7 Filip Pizlo 2015-10-05 10:06:16 PDT
Landed in http://trac.webkit.org/changeset/190561