Created attachment 296927 [details] the patch

Comment on attachment 296927 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=296927&action=review > Source/JavaScriptCore/ChangeLog:8 > + This factors out the scheduler from a bunch of lambdas and closed-over variables in Should this read "... from the scheduler a bunch ..."? > Source/JavaScriptCore/ChangeLog:14 > + still had time during a mutator timeslice. This greatly splay-latency. I'm still testing Fix the sentence "This greatly splay-latency."

Comment on attachment 296927 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=296927&action=review > Source/JavaScriptCore/heap/SpaceTimeScheduler.cpp:111 > + std::max( > + m_bytesAllocatedThisCycleAtTheBeginning, > + static_cast<double>(heap.m_maxEdenSize))) I like writing it this way: std::max<double>( ... <<no static_cast needed here>> heap.m_maxEdenSize)) > Source/JavaScriptCore/heap/SpaceTimeScheduler.h:39 > + Decision() { } This isn’t needed. If we leave it out, it gets generated with exactly the same contents as if we write this.

(In reply to comment #3) > Comment on attachment 296927 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296927&action=review > > > Source/JavaScriptCore/heap/SpaceTimeScheduler.cpp:111 > > + std::max( > > + m_bytesAllocatedThisCycleAtTheBeginning, > > + static_cast<double>(heap.m_maxEdenSize))) > > I like writing it this way: > > std::max<double>( ... <<no static_cast needed here>> heap.m_maxEdenSize)) That's so much better! I never thought to specialize max explicitly. > > > Source/JavaScriptCore/heap/SpaceTimeScheduler.h:39 > > + Decision() { } > > This isn’t needed. If we leave it out, it gets generated with exactly the > same contents as if we write this. Thanks! I always forget the exact rules. Maybe I wrote too much Java in a past life.

(In reply to comment #2) > Comment on attachment 296927 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=296927&action=review > > > Source/JavaScriptCore/ChangeLog:8 > > + This factors out the scheduler from a bunch of lambdas and closed-over variables in > > Should this read "... from the scheduler a bunch ..."? > > > Source/JavaScriptCore/ChangeLog:14 > > + still had time during a mutator timeslice. This greatly splay-latency. I'm still testing > > Fix the sentence "This greatly splay-latency." Fixed!

Landed in https://trac.webkit.org/changeset/209727

*** Bug 164940 has been marked as a duplicate of this bug. ***

(In reply to comment #6) > Landed in https://trac.webkit.org/changeset/209727 I am still confirming but this was likely a ~4% Speedometer regression on Mac.

(In reply to comment #8) > (In reply to comment #6) > > Landed in https://trac.webkit.org/changeset/209727 > > I am still confirming but this was likely a ~4% Speedometer regression on > Mac. The policy change in this patch was already rolled out.

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #6) > > > Landed in https://trac.webkit.org/changeset/209727 > > > > I am still confirming but this was likely a ~4% Speedometer regression on > > Mac. > > The policy change in this patch was already rolled out. Could you point me to the revision of the rollout please so I can confirm the progression then?

(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (In reply to comment #6) > > > > Landed in https://trac.webkit.org/changeset/209727 > > > > > > I am still confirming but this was likely a ~4% Speedometer regression on > > > Mac. > > > > The policy change in this patch was already rolled out. > > Could you point me to the revision of the rollout please so I can confirm > the progression then? https://trac.webkit.org/changeset/209763