Bug 197730 – [JSC] Compress Watchpoint size by using enum type and Packed<> data structure

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

Add Packed<> data structure to compress some frequently allocated data

[Yusuke Suzuki]

Created attachment 369460 [details]
Patch

[EWS Watchlist]

Attachment 369460 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ObjectToStringAdaptiveStructureWatchpoint.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/ObjectToStringAdaptiveStructureWatchpoint.cpp:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:50:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:52:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/Watchpoint.h:131:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:39:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:41:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Packed.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/bytecode/Watchpoint.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 369461 [details]
Patch

[EWS Watchlist]

Attachment 369461 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ObjectToStringAdaptiveStructureWatchpoint.cpp:27:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/ObjectToStringAdaptiveStructureWatchpoint.cpp:29:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:50:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:52:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/Watchpoint.h:131:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:39:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:41:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Packed.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/bytecode/Watchpoint.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 10 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 369465 [details]
Patch

[EWS Watchlist]

Attachment 369465 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:50:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:52:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:39:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:41:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 5 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 369466 [details]
Patch

[EWS Watchlist]

Attachment 369466 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:50:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:51:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:52:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:39:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp:41:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 5 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 369467 [details]
Patch

[Yusuke Suzuki]

Created attachment 369468 [details]
Patch

[Yusuke Suzuki]

Created attachment 369471 [details]
Patch

[Yusuke Suzuki]

Created attachment 369472 [details]
Patch

[Yusuke Suzuki]

Created attachment 369474 [details]
Patch

[Yusuke Suzuki]

Created attachment 369475 [details]
Patch

[Yusuke Suzuki]

Created attachment 369477 [details]
Patch

[EWS Watchlist]

Comment on attachment 369477 [details]
Patch

Attachment 369477 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12143311

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html

[EWS Watchlist]

Created attachment 369492 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Yusuke Suzuki]

Created attachment 369518 [details]
Patch

[Yusuke Suzuki]

Created attachment 369520 [details]
Patch

[Yusuke Suzuki]

Created attachment 369522 [details]
Patch

[Robin Morisset]

Comment on attachment 369522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369522&action=review

Overall I love it, I am just a bit confused by the implementation of LBS. The rest is just nits.

> Source/JavaScriptCore/ChangeLog:26
> +           Darin ARM64.

Darin => Darwin

> Source/JavaScriptCore/ChangeLog:40
> +        This is smaller in ARM64 environment.

Not clear what is smaller: the improvement or the memory usage? (I guess the latter since ARM64 pointers take less space).
Can you also give the numbers in that case? It sounds fairly easy to measure/evaluate.

> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.h:56
> +        // Own destructor may not be called. Keep members trivially destructible.

Is there a way to ensure this in C++? I am a bit wary of a correctness-critical fairly subtle property being only enforced by a comment.

> Source/JavaScriptCore/bytecode/CodeOrigin.h:237
> +    static constexpr uintptr_t s_maskCompositeValueForPointer = ((1ULL << WTF_CPU_EFFECTIVE_ADDRESS_WIDTH) - 1) & ~(8ULL - 1);

Nice! Much less error prone than what I did originally.

> Source/JavaScriptCore/bytecode/Watchpoint.cpp:67
> +        JSC_WATCHPOINT_TYPES(JSC_DEFINE_WATCHPOINT_DISPATCH)

Nit: I think this line should have one less level of indentation, as we normally don't indent cases within a switch.

> Source/WTF/wtf/MathExtras.h:695
> +template <typename T> constexpr unsigned getLSBSetConstexpr(T value)

Have you tested this function? I've got some doubts about its results for values whose least bit is set.
E.g. for 0b100001,  unsignedValue >>= 1 will be 0b10000, which is true, so the while loop will start eventually resulting in result=5 instead of 0.
Also, the result for 0 and for 1 seem to be the same, since we unconditionally shift away the least bit.

> Source/WTF/wtf/Packed.h:84
> +        std::swap(t1, t2);

I don't really see the point of this swap. Why not just do set(t2); other.set(t1); ?

> Source/WTF/wtf/Packed.h:191
> +        std::swap(t1, t2);

Same comment as above: why do we need the swap?)

[Yusuke Suzuki]

Comment on attachment 369522 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369522&action=review

>> Source/JavaScriptCore/ChangeLog:26
>> +           Darin ARM64.
> 
> Darin => Darwin

Oops, fixed.

>> Source/JavaScriptCore/ChangeLog:40
>> +        This is smaller in ARM64 environment.
> 
> Not clear what is smaller: the improvement or the memory usage? (I guess the latter since ARM64 pointers take less space).
> Can you also give the numbers in that case? It sounds fairly easy to measure/evaluate.

Memory, because of smaller size of the PackedPtr.

>> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.h:56
>> +        // Own destructor may not be called. Keep members trivially destructible.
> 
> Is there a way to ensure this in C++? I am a bit wary of a correctness-critical fairly subtle property being only enforced by a comment.

I don't have any idea to do that in a clean manner. Only ugly way works...
`std::is_trivially_destructible<DerivedClass>` does not work since the base "Watchpoint" class has non-trivial destructor.
What we want here is, std::is_trivially_destructible for the members that are added in the derived classes. We could do that if we decouple these members to one struct and perform static_assert onto that struct and putting `static_assert(std::is_trivially_destructible<DerivedClassMembers>::value, "")`.

class DerivedWatchpoint final : public Watchpoint {
    struct DerivedWatchpointMembers {
       PackedCellPtr<CodeBlock> m_codeBlock;
       ObjectPropertyCondition m_key;
    } members;
    static_assert(std::is_trivially_destructible<DerivedWatchpointMembers>::value, "");
};
And access them with `members.xxx`. But this is too ugly, and it can enlarge the size of class because alignment of DerivedWatchpointMembers can be large.
In the above example, ObjectPropretyCondition still has 8byte alignment. Even while Watchpoint's alignment is one, we miss the chance to fill the padding with m_codeBlock since DerivedWatchpointMembers's alignment becomes 8.
So I don't think we should do this.

For the classes that do not have any fields additionally, I think the only way is just adding a comment and `static_assert(sizeof(DerivedClass) == sizeof(Watchpoint), "")`.
I think the only way is adding static_assert for each property in derived classes.

>> Source/JavaScriptCore/bytecode/Watchpoint.cpp:67
>> +        JSC_WATCHPOINT_TYPES(JSC_DEFINE_WATCHPOINT_DISPATCH)
> 
> Nit: I think this line should have one less level of indentation, as we normally don't indent cases within a switch.

Fixed.

>> Source/WTF/wtf/MathExtras.h:695
>> +template <typename T> constexpr unsigned getLSBSetConstexpr(T value)
> 
> Have you tested this function? I've got some doubts about its results for values whose least bit is set.
> E.g. for 0b100001,  unsignedValue >>= 1 will be 0b10000, which is true, so the while loop will start eventually resulting in result=5 instead of 0.
> Also, the result for 0 and for 1 seem to be the same, since we unconditionally shift away the least bit.

Oh, interesting! This function is revived one from r243418 change, but it seems that this function had a bug. Added some tests and fixed.

>> Source/WTF/wtf/Packed.h:84
>> +        std::swap(t1, t2);
> 
> I don't really see the point of this swap. Why not just do set(t2); other.set(t1); ?

Yeah, we do not need to do that. Fixed.

>> Source/WTF/wtf/Packed.h:191
>> +        std::swap(t1, t2);
> 
> Same comment as above: why do we need the swap?)

Fixed.

[Robin Morisset]

> >> Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.h:56
> >> +        // Own destructor may not be called. Keep members trivially destructible.
> > 
> > Is there a way to ensure this in C++? I am a bit wary of a correctness-critical fairly subtle property being only enforced by a comment.
> 
> I don't have any idea to do that in a clean manner. Only ugly way works...
> `std::is_trivially_destructible<DerivedClass>` does not work since the base
> "Watchpoint" class has non-trivial destructor.
> What we want here is, std::is_trivially_destructible for the members that
> are added in the derived classes. We could do that if we decouple these
> members to one struct and perform static_assert onto that struct and putting
> `static_assert(std::is_trivially_destructible<DerivedClassMembers>::value,
> "")`.
> 
> class DerivedWatchpoint final : public Watchpoint {
>     struct DerivedWatchpointMembers {
>        PackedCellPtr<CodeBlock> m_codeBlock;
>        ObjectPropertyCondition m_key;
>     } members;
>    
> static_assert(std::is_trivially_destructible<DerivedWatchpointMembers>::
> value, "");
> };
> And access them with `members.xxx`. But this is too ugly, and it can enlarge
> the size of class because alignment of DerivedWatchpointMembers can be large.
> In the above example, ObjectPropretyCondition still has 8byte alignment.
> Even while Watchpoint's alignment is one, we miss the chance to fill the
> padding with m_codeBlock since DerivedWatchpointMembers's alignment becomes
> 8.
> So I don't think we should do this.
> 
> For the classes that do not have any fields additionally, I think the only
> way is just adding a comment and `static_assert(sizeof(DerivedClass) ==
> sizeof(Watchpoint), "")`.
> I think the only way is adding static_assert for each property in derived
> classes.

Thanks for the explanation, I agree that I don't see any clean way of doing it, I just had hoped I was missing something :-(.

LGTM with the LBS function fixed, but I am not yet officially a reviewer so I cannot r+.

[Yusuke Suzuki]

Created attachment 369618 [details]
Patch

[EWS Watchlist]

Attachment 369618 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/Watchpoint.cpp:67:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 369619 [details]
Patch

[EWS Watchlist]

Attachment 369619 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/Watchpoint.cpp:67:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robin Morisset]

Comment on attachment 369619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369619&action=review

> Source/WTF/wtf/MathExtras.h:714
> +constexpr unsigned getLSBSetConstexpr(T t)

What is the benefit of this function over just calling ctzConstexpr? They seem perfectly identical.

> Tools/TestWebKitAPI/Tests/WTF/MathExtras.cpp:510
> +TEST(WTF, clzConstexpr)

Thanks for all the tests!

[Yusuke Suzuki]

Comment on attachment 369619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369619&action=review

>> Source/WTF/wtf/MathExtras.h:714
>> +constexpr unsigned getLSBSetConstexpr(T t)
> 
> What is the benefit of this function over just calling ctzConstexpr? They seem perfectly identical.

We have getMSBSet / getMSBSetConstexpr, so I think the name getLSBSet / getLSBSetConstexpr offers clear meaning even while the underlying implementation is the same to ctz / ctzConstexpr.

[Yusuke Suzuki]

Created attachment 369627 [details]
Patch

[EWS Watchlist]

Attachment 369627 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/bytecode/Watchpoint.cpp:67:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 369627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369627&action=review

R=me.  I suggest just editing that comment that I responded to, so it's more clear what the issue is.

> Source/JavaScriptCore/bytecode/Watchpoint.h:95
> +// Really unfortunately, we do not have the way to dispatch appropriate destructor based on enum type.
> +// If we call destructor explicitly in the base class, it ends up calling the base destructor twice.
> +// C++20 allows this by using std::std::destroying_delete_t. But we are not using C++20 right now.

Is this really true?  I think what you're saying is that the base class's destructor can't do dispatch to a subclass's destructor.  But the first sentence "Really unfortunately, ... enum type." makes it seem like you're saying that there is no way to do this at all.

I guess if we had to we would do this:

- calling Watchpoint::~Watchpoint is illegal (unless Watchpoint could be used as a concrete implementation of Watchpoint, which I think isn't the case).
- doing `delete watchpoint` is illegal.
- you have to delete watchpoints by saying watchpoint->destroy(), and Watchpoint::destroy() does the dispatch.
- concrete implementations of Watchpoint would have a final destructor, and you could say `delete concreteWatchpointSubtypeInstance`.

> Source/JavaScriptCore/bytecode/Watchpoint.h:100
> +// Luckily, none of our derived watchpoint classes have members which require destructors. So now we do
> +// not dispatch the destructor call to the drived class. If it becomes really required, we can introduce
> +// a custom deleter to container classes (Bag, Vector etc.) and call Watchpoint::destroy instead of "delete"
> +// operator. But since we do not require it for now, we are doing the simplest thing.

Right - but why would we need a custom deleter in container classes?

A Vector<> would never hold Watchpoint in it, unless Watchpoint could be used as a concrete implementation of Watchpoint, which I think isn't the case.  You could do Vector<ConcreteWatchpointSubtype>, but then that would Just Work.

I think ditto for Bag.

We would have to specialize RefPtr and unique_ptr, though.

> Source/JavaScriptCore/bytecode/Watchpoint.h:130
> +#define JSC_WATCHPOINT_FIELD(type, member) \
> +    type member; \
> +    static_assert(std::is_trivially_destructible<type>::value, ""); \

Nice.

[Yusuke Suzuki]

Comment on attachment 369627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369627&action=review

Thanks, I fixed the comments :)

>> Source/JavaScriptCore/bytecode/Watchpoint.h:95
>> +// C++20 allows this by using std::std::destroying_delete_t. But we are not using C++20 right now.
> 
> Is this really true?  I think what you're saying is that the base class's destructor can't do dispatch to a subclass's destructor.  But the first sentence "Really unfortunately, ... enum type." makes it seem like you're saying that there is no way to do this at all.
> 
> I guess if we had to we would do this:
> 
> - calling Watchpoint::~Watchpoint is illegal (unless Watchpoint could be used as a concrete implementation of Watchpoint, which I think isn't the case).
> - doing `delete watchpoint` is illegal.
> - you have to delete watchpoints by saying watchpoint->destroy(), and Watchpoint::destroy() does the dispatch.
> - concrete implementations of Watchpoint would have a final destructor, and you could say `delete concreteWatchpointSubtypeInstance`.

Yeah, what we cannot do is that dispatching appropriate destructor in Base class' ~Base(). If we can do that appropriately, we can offer the class that works completely in all the use case.
But we cannot do that (until C++20), so as you described, we have some limitations.

- calling Watchpoint::~Watchpoint is illegal (unless Watchpoint could be used as a concrete implementation of Watchpoint, which I think isn't the case).

Yes, and I think we can do that immediately now :), just putting this behind "protected:". I've just done this to make Watchpoint safer.

- doing `delete watchpoint` is illegal.

Yes, this means that the following thing requires a custom deleter.

std::unique_ptr<Watchpoint> watchpoint = SomeWatchpint::create();

- you have to delete watchpoints by saying watchpoint->destroy(), and Watchpoint::destroy() does the dispatch.

Yup.

- concrete implementations of Watchpoint would have a final destructor, and you could say `delete concreteWatchpointSubtypeInstance`.

Right.

>> Source/JavaScriptCore/bytecode/Watchpoint.h:100
>> +// operator. But since we do not require it for now, we are doing the simplest thing.
> 
> Right - but why would we need a custom deleter in container classes?
> 
> A Vector<> would never hold Watchpoint in it, unless Watchpoint could be used as a concrete implementation of Watchpoint, which I think isn't the case.  You could do Vector<ConcreteWatchpointSubtype>, but then that would Just Work.
> 
> I think ditto for Bag.
> 
> We would have to specialize RefPtr and unique_ptr, though.

Right. Vector/Bag is wrong, what we need is deleter for RefPtr / unique_ptr.

[Yusuke Suzuki]

Committed r245214: <https://trac.webkit.org/changeset/245214>

[Radar WebKit Bug Importer]

<rdar://problem/50708883>

[Saam Barati]

Comment on attachment 369627 [details]
Patch

LGTM too

[Sam Weinig]

Comment on attachment 369627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369627&action=review

> Source/WTF/wtf/Platform.h:759
> +#if CPU(ADDRESS64)
> +#if OS(DARWIN) && CPU(ARM64)
> +#define WTF_CPU_EFFECTIVE_ADDRESS_WIDTH 36
> +#else
> +/* We strongly assume that effective address width is <= 48 in 64bit architectures (e.g. NaN boxing). */
> +#define WTF_CPU_EFFECTIVE_ADDRESS_WIDTH 48
> +#endif
> +#else
> +#define WTF_CPU_EFFECTIVE_ADDRESS_WIDTH 32
> +#endif

On Darwin at least, we should be able to statically assert these constants (or even derive them) from MACH_VM_MAX_ADDRESS (in vm_param.h)

[Yusuke Suzuki]

Comment on attachment 369627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369627&action=review

>> Source/WTF/wtf/Platform.h:759
>> +#endif
> 
> On Darwin at least, we should be able to statically assert these constants (or even derive them) from MACH_VM_MAX_ADDRESS (in vm_param.h)

Sounds nice. I've added the static_assert to WTFAssertions.cpp.

[Yusuke Suzuki]

Committed r245254: <https://trac.webkit.org/changeset/245254>

[Yusuke Suzuki]

Committed r245264: <https://trac.webkit.org/changeset/245264>

[Sam Weinig]

(In reply to Yusuke Suzuki from comment #40)
> Committed r245264: <https://trac.webkit.org/changeset/245264>

Awesome! Thanks Yusuke.