Bug 234628 – Make DeferredWorkTimer::addPendingWork() return a Ticket.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

This is so that we can use the compiler to enforce that we always call DeferredWorkTimer::addPendingWork() before calling DeferredWorkTimer::scheduleWorkSoon().

rdar://84260429

[Mark Lam]

Created attachment 447855 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 447855 [details]
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.

[Mark Lam]

Thanks for the review.

(In reply to Yusuke Suzuki from comment #2)
> You can use `HashMap::removeIf` / `HashSet::removeIf`, which is more
> efficient.

Fixed.

> > 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.

I can't do this yet.  Will need to add 2 APIs: FixedVector::clear() and FixedVector::contains().  It's not difficult to add these, but I'll do that in a separate patch, and make this change after that.

> > 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.

Fixed.

> > 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.

Fixed.

[Mark Lam]

Created attachment 447899 [details]
patch for landing.

[Mark Lam]

Created attachment 447905 [details]
[fast-cq] patch for landing.

[Mark Lam]

Comment on attachment 447905 [details]
[fast-cq] patch for landing.

Landing now.

[EWS]

Committed r287421 (245556@main): <https://commits.webkit.org/245556@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447905 [details].

[Mark Lam]

(In reply to Mark Lam from comment #3)
> (In reply to Yusuke Suzuki from comment #2)
> > > 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.
> 
> I can't do this yet.  Will need to add 2 APIs: FixedVector::clear() and
> FixedVector::contains().  It's not difficult to add these, but I'll do that
> in a separate patch, and make this change after that.

Fixing this in https://bugs.webkit.org/show_bug.cgi?id=234855.