GC scheduler should avoid consecutive pauses

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 296927 [details]
the patch

[Michael Saboff]

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

[Darin Adler]

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.

[Filip Pizlo]

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

[Filip Pizlo]

(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!

[Filip Pizlo]

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

[Filip Pizlo]

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

[Chris Dumez]

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

[Filip Pizlo]

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

[Chris Dumez]

(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?

[Filip Pizlo]

(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