Bug 204149 – [JSC] BlockDirectory's bits should be compact

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 383442 [details]
Patch

[Yusuke Suzuki]

Created attachment 383443 [details]
Patch

[Yusuke Suzuki]

Created attachment 383445 [details]
Patch

[Yusuke Suzuki]

Created attachment 383446 [details]
Patch

[Yusuke Suzuki]

Created attachment 383447 [details]
Patch

[Mark Lam]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 383492 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 383495 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 383500 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 383507 [details]
Patch

[Yusuke Suzuki]

Created attachment 383514 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 383518 [details]
Patch

[Yusuke Suzuki]

Created attachment 383519 [details]
Patch

[Yusuke Suzuki]

Created attachment 383520 [details]
Patch

[Yusuke Suzuki]

Created attachment 383521 [details]
Patch

[Yusuke Suzuki]

Created attachment 383528 [details]
Patch

[Yusuke Suzuki]

Created attachment 383532 [details]
Patch

[Robin Morisset]

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>

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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 :)

[Yusuke Suzuki]

Committed r252452: <https://trac.webkit.org/changeset/252452>

[Radar WebKit Bug Importer]

<rdar://problem/57184145>