Bug 197730 - [JSC] Compress Watchpoint size by using enum type and Packed<> data structure
Summary: [JSC] Compress Watchpoint size by using enum type and Packed<> data structure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-08 21:05 PDT by Yusuke Suzuki
Modified: 2019-05-13 17:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (60.00 KB, patch)
2019-05-08 21:09 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (60.02 KB, patch)
2019-05-08 21:19 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (53.34 KB, patch)
2019-05-08 22:49 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (60.96 KB, patch)
2019-05-08 23:00 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (64.10 KB, patch)
2019-05-08 23:10 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (62.30 KB, patch)
2019-05-08 23:13 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (62.51 KB, patch)
2019-05-08 23:57 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (64.85 KB, patch)
2019-05-09 00:30 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (68.39 KB, patch)
2019-05-09 00:41 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (68.46 KB, patch)
2019-05-09 00:53 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (69.00 KB, patch)
2019-05-09 01:21 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.66 MB, application/zip)
2019-05-09 07:30 PDT, Build Bot
no flags Details
Patch (74.92 KB, patch)
2019-05-09 13:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (75.01 KB, patch)
2019-05-09 14:00 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (75.15 KB, patch)
2019-05-09 14:34 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (82.20 KB, patch)
2019-05-10 16:28 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (80.69 KB, patch)
2019-05-10 16:33 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (80.76 KB, patch)
2019-05-10 17:57 PDT, Yusuke Suzuki
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-05-08 21:05:46 PDT
Add Packed<> data structure to compress some frequently allocated data
Comment 1 Yusuke Suzuki 2019-05-08 21:09:20 PDT
Created attachment 369460 [details]
Patch
Comment 2 Build Bot 2019-05-08 21:12:10 PDT
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.
Comment 3 Yusuke Suzuki 2019-05-08 21:19:14 PDT
Created attachment 369461 [details]
Patch
Comment 4 Build Bot 2019-05-08 21:20:30 PDT
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.
Comment 5 Yusuke Suzuki 2019-05-08 22:49:00 PDT
Created attachment 369465 [details]
Patch
Comment 6 Build Bot 2019-05-08 22:51:03 PDT
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.
Comment 7 Yusuke Suzuki 2019-05-08 23:00:02 PDT
Created attachment 369466 [details]
Patch
Comment 8 Build Bot 2019-05-08 23:03:50 PDT
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.
Comment 9 Yusuke Suzuki 2019-05-08 23:10:07 PDT
Created attachment 369467 [details]
Patch
Comment 10 Yusuke Suzuki 2019-05-08 23:13:27 PDT
Created attachment 369468 [details]
Patch
Comment 11 Yusuke Suzuki 2019-05-08 23:57:24 PDT
Created attachment 369471 [details]
Patch
Comment 12 Yusuke Suzuki 2019-05-09 00:30:26 PDT
Created attachment 369472 [details]
Patch
Comment 13 Yusuke Suzuki 2019-05-09 00:41:29 PDT
Created attachment 369474 [details]
Patch
Comment 14 Yusuke Suzuki 2019-05-09 00:53:05 PDT
Created attachment 369475 [details]
Patch
Comment 15 Yusuke Suzuki 2019-05-09 01:21:02 PDT
Created attachment 369477 [details]
Patch
Comment 16 Build Bot 2019-05-09 07:30:14 PDT
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
Comment 17 Build Bot 2019-05-09 07:30:16 PDT
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
Comment 18 Yusuke Suzuki 2019-05-09 13:17:22 PDT
Created attachment 369518 [details]
Patch
Comment 19 Yusuke Suzuki 2019-05-09 14:00:58 PDT
Created attachment 369520 [details]
Patch
Comment 20 Yusuke Suzuki 2019-05-09 14:34:02 PDT
Created attachment 369522 [details]
Patch
Comment 21 Robin Morisset 2019-05-10 10:56:10 PDT
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?)
Comment 22 Yusuke Suzuki 2019-05-10 11:36:25 PDT
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.
Comment 23 Robin Morisset 2019-05-10 13:13:58 PDT
> >> 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+.
Comment 24 Yusuke Suzuki 2019-05-10 16:28:28 PDT
Created attachment 369618 [details]
Patch
Comment 25 Build Bot 2019-05-10 16:33:09 PDT
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.
Comment 26 Yusuke Suzuki 2019-05-10 16:33:19 PDT
Created attachment 369619 [details]
Patch
Comment 27 Build Bot 2019-05-10 16:36:36 PDT
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.
Comment 28 Robin Morisset 2019-05-10 16:51:43 PDT
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!
Comment 29 Yusuke Suzuki 2019-05-10 16:54:41 PDT
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.
Comment 30 Yusuke Suzuki 2019-05-10 17:57:41 PDT
Created attachment 369627 [details]
Patch
Comment 31 Build Bot 2019-05-10 17:59:19 PDT
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.
Comment 32 Filip Pizlo 2019-05-12 13:27:06 PDT
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.
Comment 33 Yusuke Suzuki 2019-05-12 15:44:41 PDT
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.
Comment 34 Yusuke Suzuki 2019-05-12 15:50:26 PDT
Committed r245214: <https://trac.webkit.org/changeset/245214>
Comment 35 Radar WebKit Bug Importer 2019-05-12 15:52:33 PDT
<rdar://problem/50708883>
Comment 36 Saam Barati 2019-05-13 12:07:14 PDT
Comment on attachment 369627 [details]
Patch

LGTM too
Comment 37 Sam Weinig 2019-05-13 14:05:06 PDT
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)
Comment 38 Yusuke Suzuki 2019-05-13 14:43:58 PDT
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.
Comment 39 Yusuke Suzuki 2019-05-13 15:00:32 PDT
Committed r245254: <https://trac.webkit.org/changeset/245254>
Comment 40 Yusuke Suzuki 2019-05-13 16:43:30 PDT
Committed r245264: <https://trac.webkit.org/changeset/245264>
Comment 41 Sam Weinig 2019-05-13 17:28:38 PDT
(In reply to Yusuke Suzuki from comment #40)
> Committed r245264: <https://trac.webkit.org/changeset/245264>

Awesome! Thanks Yusuke.