Bug 149796 - Inline cache repatching should be throttled if it happens a lot
Summary: Inline cache repatching should be throttled if it happens a lot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-04 12:11 PDT by Filip Pizlo
Modified: 2015-10-05 10:06 PDT (History)
14 users (show)

See Also:


Attachments
the patch (28.96 KB, patch)
2015-10-04 12:24 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (30.51 KB, patch)
2015-10-04 12:26 PDT, Filip Pizlo
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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