[webkit-reviews] review granted: [Bug 197730] [JSC] Compress Watchpoint size by using enum type and Packed<> data structure : [Attachment 369627] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 197730: [JSC] Compress Watchpoint size by using enum type and Packed<> data
structure
https://bugs.webkit.org/show_bug.cgi?id=197730

Attachment 369627: Patch

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




--- Comment #32 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 369627
  --> https://bugs.webkit.org/attachment.cgi?id=369627
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.