[webkit-reviews] review granted: [Bug 234628] Make DeferredWorkTimer::addPendingWork() return a Ticket. : [Attachment 447855] proposed patch.

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 234628: Make DeferredWorkTimer::addPendingWork() return a Ticket.
https://bugs.webkit.org/show_bug.cgi?id=234628

Attachment 447855: proposed patch.

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




--- Comment #2 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 447855
  --> https://bugs.webkit.org/attachment.cgi?id=447855
proposed patch.

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

r=me

> Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp:142
> +    Vector<Ticket, 16> cancelledTickets;
> +    for (auto& it : m_pendingTickets) {
> +	   if (it.value->isCancelled())
> +	       cancelledTickets.append(it.key);
> +    }
> +    for (auto& ticket : cancelledTickets)
> +	   m_pendingTickets.remove(ticket);

You can use `HashMap::removeIf` / `HashSet::removeIf`, which is more efficient.

> Source/JavaScriptCore/runtime/DeferredWorkTimer.h:58
>	   Vector<Strong<JSCell>> dependencies;

It is good if we can use FixedVector<Strong<JSCell>> here, which allocates
exact size, and it only has one pointer size.

> Source/JavaScriptCore/runtime/DeferredWorkTimer.h:92
> +    HashMap<Ticket, std::unique_ptr<TicketData>> m_pendingTickets;

Why not using `HashSet<std::unique_ptr<TicketData>>`? I think, because of
GetPtr abstraction, we can still look up from it via TicketData*, a.k.a.
Ticket.
And HashSet is 2x more space efficient.

> Source/JavaScriptCore/wasm/WasmStreamingCompiler.cpp:210
> +    // The pending work TicketData was keeping the promise alive. We  need
to

Remove one space between We and need.