Summary: | [JSC] BlockDirectory's bits should be compact | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, benjamin, cdumez, cmarcelo, dbates, ews-watchlist, gyuyoung.kim, keith_miller, mark.lam, msaboff, rmorisset, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer | ||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Yusuke Suzuki
2019-11-13 02:26:05 PST
Created attachment 383442 [details]
Patch
Created attachment 383443 [details]
Patch
Created attachment 383445 [details]
Patch
Created attachment 383446 [details]
Patch
Created attachment 383447 [details]
Patch
Comment on attachment 383447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383447&action=review > Source/JavaScriptCore/heap/BlockDirectory.cpp:351 > + [&] (auto, const char* name) { Are you intentionally passing by value here? Comment on attachment 383447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383447&action=review >> Source/JavaScriptCore/heap/BlockDirectory.cpp:351 >> + [&] (auto, const char* name) { > > Are you intentionally passing by value here? Yes, this should be BlockDirectoryBitVectorRef. But I'm planning to change this to BlockDirectoryBitVectorRef to make it explicit. Comment on attachment 383447 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383447&action=review >>> Source/JavaScriptCore/heap/BlockDirectory.cpp:351 >>> + [&] (auto, const char* name) { >> >> Are you intentionally passing by value here? > > Yes, this should be BlockDirectoryBitVectorRef. But I'm planning to change this to BlockDirectoryBitVectorRef to make it explicit. Ah, we cannot. Each bitvector has different type. Created attachment 383492 [details]
Patch
WIP
Created attachment 383495 [details]
Patch
WIP
Created attachment 383500 [details]
Patch
WIP
Created attachment 383507 [details]
Patch
Created attachment 383514 [details]
Patch
WIP
Created attachment 383518 [details]
Patch
Created attachment 383519 [details]
Patch
Created attachment 383520 [details]
Patch
Created attachment 383521 [details]
Patch
Created attachment 383528 [details]
Patch
Created attachment 383532 [details]
Patch
Comment on attachment 383532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383532&action=review r=me > Source/JavaScriptCore/heap/BlockDirectoryBits.h:156 > + for (unsigned i = BlockDirectoryBitVectorView<kind>::arrayLength(); i--;) Is there a reason for the previous function using Base::arrayLength() while this one is using BlockDirectoryBitVectorView<kind>::arrayLength() ? > Source/JavaScriptCore/heap/BlockDirectoryBits.h:191 > + BlockDirectoryBitVectorRef<Kind::capitalBitName> lowerBitName() \ I don't think this function is useful as long as you have the one above. Either delete it, or make it return a const BlockDirectoryBitVectorRef<Kind::capitalBitName> Comment on attachment 383532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383532&action=review Thanks! >> Source/JavaScriptCore/heap/BlockDirectoryBits.h:156 >> + for (unsigned i = BlockDirectoryBitVectorView<kind>::arrayLength(); i--;) > > Is there a reason for the previous function using Base::arrayLength() while this one is using BlockDirectoryBitVectorView<kind>::arrayLength() ? No reason. Thanks, fixed. >> Source/JavaScriptCore/heap/BlockDirectoryBits.h:191 >> + BlockDirectoryBitVectorRef<Kind::capitalBitName> lowerBitName() \ > > I don't think this function is useful as long as you have the one above. > Either delete it, or make it return a const BlockDirectoryBitVectorRef<Kind::capitalBitName> Yeah, removing View version, since Ref one is used to modify the bits. Comment on attachment 383532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383532&action=review >>> Source/JavaScriptCore/heap/BlockDirectoryBits.h:191 >>> + BlockDirectoryBitVectorRef<Kind::capitalBitName> lowerBitName() \ >> >> I don't think this function is useful as long as you have the one above. >> Either delete it, or make it return a const BlockDirectoryBitVectorRef<Kind::capitalBitName> > > Yeah, removing View version, since Ref one is used to modify the bits. I tried removing this but it brings much complexity like implementing `bool operator[] const`. For now, I'll just keep it since it just works anyway :) Committed r252452: <https://trac.webkit.org/changeset/252452> |