Bug 204149

Summary: [JSC] BlockDirectory's bits should be compact
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch rmorisset: review+

Description Yusuke Suzuki 2019-11-13 02:26:05 PST
...
Comment 1 Yusuke Suzuki 2019-11-13 02:30:18 PST
Created attachment 383442 [details]
Patch
Comment 2 Yusuke Suzuki 2019-11-13 02:55:36 PST
Created attachment 383443 [details]
Patch
Comment 3 Yusuke Suzuki 2019-11-13 03:03:25 PST
Created attachment 383445 [details]
Patch
Comment 4 Yusuke Suzuki 2019-11-13 03:15:58 PST
Created attachment 383446 [details]
Patch
Comment 5 Yusuke Suzuki 2019-11-13 03:23:19 PST
Created attachment 383447 [details]
Patch
Comment 6 Mark Lam 2019-11-13 09:39:29 PST
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 7 Yusuke Suzuki 2019-11-13 12:35:08 PST
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 8 Yusuke Suzuki 2019-11-13 12:51:06 PST
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.
Comment 9 Yusuke Suzuki 2019-11-13 13:53:31 PST
Created attachment 383492 [details]
Patch

WIP
Comment 10 Yusuke Suzuki 2019-11-13 14:12:05 PST
Created attachment 383495 [details]
Patch

WIP
Comment 11 Yusuke Suzuki 2019-11-13 14:52:40 PST
Created attachment 383500 [details]
Patch

WIP
Comment 12 Yusuke Suzuki 2019-11-13 16:02:13 PST
Created attachment 383507 [details]
Patch
Comment 13 Yusuke Suzuki 2019-11-13 16:31:11 PST
Created attachment 383514 [details]
Patch

WIP
Comment 14 Yusuke Suzuki 2019-11-13 16:49:36 PST
Created attachment 383518 [details]
Patch
Comment 15 Yusuke Suzuki 2019-11-13 16:56:12 PST
Created attachment 383519 [details]
Patch
Comment 16 Yusuke Suzuki 2019-11-13 17:03:06 PST
Created attachment 383520 [details]
Patch
Comment 17 Yusuke Suzuki 2019-11-13 17:14:01 PST
Created attachment 383521 [details]
Patch
Comment 18 Yusuke Suzuki 2019-11-13 17:49:43 PST
Created attachment 383528 [details]
Patch
Comment 19 Yusuke Suzuki 2019-11-13 18:01:42 PST
Created attachment 383532 [details]
Patch
Comment 20 Robin Morisset 2019-11-13 18:36:59 PST
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 21 Yusuke Suzuki 2019-11-13 19:00:42 PST
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 22 Yusuke Suzuki 2019-11-13 19:08:07 PST
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 :)
Comment 23 Yusuke Suzuki 2019-11-14 01:37:16 PST
Committed r252452: <https://trac.webkit.org/changeset/252452>
Comment 24 Radar WebKit Bug Importer 2019-11-14 01:38:17 PST
<rdar://problem/57184145>