Summary: | Inline cache repatching should be throttled if it happens a lot | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||
Component: | JavaScriptCore | Assignee: | 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
Filip Pizlo
2015-10-04 12:11:53 PDT
Created attachment 262405 [details]
the patch
Created attachment 262406 [details]
the patch
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 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 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" (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. Landed in http://trac.webkit.org/changeset/190561 |