Bug 234628 - Make DeferredWorkTimer::addPendingWork() return a Ticket.
Summary: Make DeferredWorkTimer::addPendingWork() return a Ticket.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-22 20:16 PST by Mark Lam
Modified: 2022-01-04 11:59 PST (History)
7 users (show)

See Also:


Attachments
proposed patch. (29.91 KB, patch)
2021-12-22 20:19 PST, Mark Lam
ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing. (30.30 KB, patch)
2021-12-23 12:23 PST, Mark Lam
no flags Details | Formatted Diff | Diff
[fast-cq] patch for landing. (30.93 KB, patch)
2021-12-23 13:58 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2021-12-22 20:16:52 PST
This is so that we can use the compiler to enforce that we always call DeferredWorkTimer::addPendingWork() before calling DeferredWorkTimer::scheduleWorkSoon().

rdar://84260429
Comment 1 Mark Lam 2021-12-22 20:19:54 PST
Created attachment 447855 [details]
proposed patch.
Comment 2 Yusuke Suzuki 2021-12-22 20:35:32 PST
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.
Comment 3 Mark Lam 2021-12-23 12:02:12 PST
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.
Comment 4 Mark Lam 2021-12-23 12:23:51 PST
Created attachment 447899 [details]
patch for landing.
Comment 5 Mark Lam 2021-12-23 13:58:57 PST
Created attachment 447905 [details]
[fast-cq] patch for landing.
Comment 6 Mark Lam 2021-12-23 21:56:52 PST
Comment on attachment 447905 [details]
[fast-cq] patch for landing.

Landing now.
Comment 7 EWS 2021-12-23 22:00:09 PST
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].
Comment 8 Mark Lam 2022-01-04 11:59:32 PST
(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.