Bug 167528 – The mutator should be able to perform increments of GC work

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

If you don't have a lot of cores, then running the GC in a purely concurrent way can be bad: you won't make enough progress to keep pace with the mutator's allocations.

Fortunately, Riptide has most of what we need to run increments of GC work on the mutator thread. This is the most natural way to pace the mutator, since it forces the mutator to do GC work anytime it allocates.

[Filip Pizlo]

Created attachment 299962 [details]
work in progress

[Filip Pizlo]

Created attachment 300158 [details]
possibly better version

[Filip Pizlo]

Created attachment 300235 [details]
the patch

[Keith Miller]

Comment on attachment 300235 [details]
the patch

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

r=me.

> Source/JavaScriptCore/heap/SlotVisitor.cpp:552
> +            if (status == IterationStatus::Continue)
> +                break;

Using IterationStatus::Continue to signal we have no more work to do is confusing but I guess that's the pattern.

> Source/WTF/wtf/DataLog.cpp:131
> +    s_file = new (s_lockedFileData) LockedPrintStream(std::unique_ptr<FilePrintStream>(file));

oh wow...

[Filip Pizlo]

(In reply to comment #4)
> Comment on attachment 300235 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300235&action=review
> 
> r=me.
> 
> > Source/JavaScriptCore/heap/SlotVisitor.cpp:552
> > +            if (status == IterationStatus::Continue)
> > +                break;
> 
> Using IterationStatus::Continue to signal we have no more work to do is
> confusing but I guess that's the pattern.

It's unfortunate. I will we had generators. It's actually so confusing here: we say "Continue" to tell the forEachBlah to go to the next mark stack. We do that when the mark stack we were at had no work. So, if forEachBlah returns Continue then no mark stack had work. Hence, if it says Continue, we break.

> 
> > Source/WTF/wtf/DataLog.cpp:131
> > +    s_file = new (s_lockedFileData) LockedPrintStream(std::unique_ptr<FilePrintStream>(file));
> 
> oh wow...

[Geoffrey Garen]

Comment on attachment 300235 [details]
the patch

r=me

> Source/JavaScriptCore/ChangeLog:24
> +        precise at keeping case as the new work-based incremental mode.

keeping pace

> Source/JavaScriptCore/ChangeLog:33
> +        each other is a huge splay-latency speed-up on my iPad. It's also a CDjs progression. It does
> +        progress splay-throughput, but I think that's fine (the regression is 11%, the progression is

did you mean *regress* splay-throughput?

[Filip Pizlo]

(In reply to comment #6)
> Comment on attachment 300235 [details]
> the patch
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:24
> > +        precise at keeping case as the new work-based incremental mode.
> 
> keeping pace

Fixed.

> 
> > Source/JavaScriptCore/ChangeLog:33
> > +        each other is a huge splay-latency speed-up on my iPad. It's also a CDjs progression. It does
> > +        progress splay-throughput, but I think that's fine (the regression is 11%, the progression is
> 
> did you mean *regress* splay-throughput?

Yes. Fixed.

[Filip Pizlo]

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