Bug 171708 – GCController.cpp's collect() should be Async

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 309128 [details]
the patch

[Saam Barati]

Comment on attachment 309128 [details]
the patch

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

> Source/WebCore/bindings/js/GCController.cpp:44
> +    commonVM().heap.collectNow(Async, CollectionScope::Full);

nit: I still think this should've been an enum class. It scares me that we have top level Sync/Async enum values in JSC.

[Filip Pizlo]

(In reply to Saam Barati from comment #2)
> Comment on attachment 309128 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309128&action=review
> 
> > Source/WebCore/bindings/js/GCController.cpp:44
> > +    commonVM().heap.collectNow(Async, CollectionScope::Full);
> 
> nit: I still think this should've been an enum class. It scares me that we
> have top level Sync/Async enum values in JSC.

I didn't realize you felt that strongly about it!

What do you think is the downside of having top-level Sync/Async enum values in JSC?

[Filip Pizlo]

I'll wait until tomorrow to land this.  I want the bots to see it separately from the previous change I just landed.

[Saam Barati]

(In reply to Filip Pizlo from comment #3)
> (In reply to Saam Barati from comment #2)
> > Comment on attachment 309128 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=309128&action=review
> > 
> > > Source/WebCore/bindings/js/GCController.cpp:44
> > > +    commonVM().heap.collectNow(Async, CollectionScope::Full);
> > 
> > nit: I still think this should've been an enum class. It scares me that we
> > have top level Sync/Async enum values in JSC.
> 
> I didn't realize you felt that strongly about it!
> 
> What do you think is the downside of having top-level Sync/Async enum values
> in JSC?

I guess the names worry me less than just having the extra type safety than enum class buys you. I'm more OK with raw enums when they're embedded in a class because their scope is more limited, but even then I tend to prefer enum class since it's almost pure win from a type perspective. Raw enums inside a namespace feel less contained than in a class.

All this said, I don't feel that strongly. I definitely have a preference, but it's not a deal breaker for me.

[Build Bot]

Comment on attachment 309128 [details]
the patch

Attachment 309128 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3676409

New failing tests:
fast/dom/Window/remove-timeout-crash.html

[Build Bot]

Created attachment 309139 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Filip Pizlo]

@Youenn: my patch changes the timing of some destructors, and this seems to cause a crash in RealtimeMediaSourceCenter as a result.

Have you seen this before?

[Filip Pizlo]

Here's the crash log.

If it proves hard to repro, I'm going to assume that this is a pre-existing condition.


ASSERTION FAILED: wasRemoved
/Volumes/Data/EWS/WebKit/Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp(171) : void WebCore::RealtimeMediaSourceCenter::removeDevicesChangedObserver(DevicesChangedObserverToken)
1   0x10e8bf180 WTFCrash
2   0x117eac599 WebCore::RealtimeMediaSourceCenter::removeDevicesChangedObserver(unsigned int)
3   0x117af7059 WebCore::MediaDevices::~MediaDevices()
4   0x117af70c5 WebCore::MediaDevices::~MediaDevices()
5   0x117af7119 WebCore::MediaDevices::~MediaDevices()
6   0x1173c0713 WTF::RefCounted<WebCore::MediaDevices>::deref() const
7   0x1173c06b7 WTF::Ref<WebCore::MediaDevices>::~Ref()
8   0x1173c0675 WTF::Ref<WebCore::MediaDevices>::~Ref()
9   0x1173c064c WebCore::JSDOMWrapper<WebCore::MediaDevices>::~JSDOMWrapper()
10  0x1173c0625 WebCore::JSMediaDevices::~JSMediaDevices()
11  0x1173bbd35 WebCore::JSMediaDevices::~JSMediaDevices()
12  0x1173b9f7d WebCore::JSMediaDevices::destroy(JSC::JSCell*)
13  0x10e25b35a JSC::(anonymous namespace)::DestroyFunc::operator()(JSC::VM&, JSC::JSCell*) const
14  0x10e25cffb JSC::FreeList JSC::MarkedBlock::Handle::specializedSweep<false, (JSC::MarkedBlock::Handle::EmptyMode)0, (JSC::MarkedBlock::Handle::SweepMode)0, (JSC::MarkedBlock::Handle::SweepDestructionMode)0, (JSC::MarkedBlock::Handle::ScribbleMode)0, (JSC::MarkedBlock::Handle::NewlyAllocatedMode)0, (JSC::MarkedBlock::Handle::MarksMode)0, JSC::(anonymous namespace)::DestroyFunc>(JSC::MarkedBlock::Handle::EmptyMode, JSC::MarkedBlock::Handle::SweepMode, JSC::MarkedBlock::Handle::SweepDestructionMode, JSC::MarkedBlock::Handle::ScribbleMode, JSC::MarkedBlock::Handle::NewlyAllocatedMode, JSC::MarkedBlock::Handle::MarksMode, JSC::(anonymous namespace)::DestroyFunc const&)::'lambda'(unsigned long)::operator()(unsigned long) const
15  0x10e25b9cf JSC::FreeList JSC::MarkedBlock::Handle::specializedSweep<false, (JSC::MarkedBlock::Handle::EmptyMode)0, (JSC::MarkedBlock::Handle::SweepMode)0, (JSC::MarkedBlock::Handle::SweepDestructionMode)0, (JSC::MarkedBlock::Handle::ScribbleMode)0, (JSC::MarkedBlock::Handle::NewlyAllocatedMode)0, (JSC::MarkedBlock::Handle::MarksMode)0, JSC::(anonymous namespace)::DestroyFunc>(JSC::MarkedBlock::Handle::EmptyMode, JSC::MarkedBlock::Handle::SweepMode, JSC::MarkedBlock::Handle::SweepDestructionMode, JSC::MarkedBlock::Handle::ScribbleMode, JSC::MarkedBlock::Handle::NewlyAllocatedMode, JSC::MarkedBlock::Handle::MarksMode, JSC::(anonymous namespace)::DestroyFunc const&)
16  0x10e25b2df JSC::FreeList JSC::MarkedBlock::Handle::finishSweepKnowingSubspace<JSC::(anonymous namespace)::DestroyFunc>(JSC::MarkedBlock::Handle::SweepMode, JSC::(anonymous namespace)::DestroyFunc const&)
17  0x10e25b15d JSC::JSDestructibleObjectSubspace::finishSweep(JSC::MarkedBlock::Handle&, JSC::MarkedBlock::Handle::SweepMode)
18  0x10e41e4ff JSC::MarkedBlock::Handle::sweep(JSC::MarkedBlock::Handle::SweepMode)
19  0x10e0aa647 JSC::IncrementalSweeper::sweepNextBlock()
20  0x10e0aa4e2 JSC::IncrementalSweeper::doSweep(WTF::MonotonicTime)
21  0x10e0aa4ac JSC::IncrementalSweeper::doWork()
22  0x10df07831 JSC::JSRunLoopTimer::timerDidFire()
23  0x10df07b7c JSC::JSRunLoopTimer::timerDidFireCallback(__CFRunLoopTimer*, void*)
24  0x7fff9f631af4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
25  0x7fff9f631783 __CFRunLoopDoTimer
26  0x7fff9f6312da __CFRunLoopDoTimers
27  0x7fff9f6287d1 __CFRunLoopRun
28  0x7fff9f627e38 CFRunLoopRunSpecific
29  0x10ce76ec3 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)
30  0x10ce7554d runTestingServerLoop()
31  0x10ce74ad2 dumpRenderTree(int, char const**)

[Filip Pizlo]

Created attachment 309173 [details]
patch for landing

Try EWS again.

[youenn fablet]

I think it is a preexisting issue, ccing jer and eric on that.

[Eric Carlson]

Yes: https://bugs.webkit.org/show_bug.cgi?id=171529

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/216262/webkit

[Saam Barati]

(In reply to Filip Pizlo from comment #13)
> Landed in http://trac.webkit.org/changeset/216262/webkit

This appears to be a 7% iOS JetStream regression. Can we roll it out?

[Filip Pizlo]

(In reply to Saam Barati from comment #14)
> (In reply to Filip Pizlo from comment #13)
> > Landed in http://trac.webkit.org/changeset/216262/webkit
> 
> This appears to be a 7% iOS JetStream regression. Can we roll it out?

Yes, but we might want to consider keeping the regression if the regressed tests were previously benefiting from their GC work happening in between measurement iterations. It's not in the spirit of the test, or of WebKit, to hide GC work from benchmarks. 

Do you know what tests are regressed?

[Filip Pizlo]

But anyway, ok to roll out - we can investigate this after the regression is recovered.

[Saam Barati]

(In reply to Filip Pizlo from comment #15)
> (In reply to Saam Barati from comment #14)
> > (In reply to Filip Pizlo from comment #13)
> > > Landed in http://trac.webkit.org/changeset/216262/webkit
> > 
> > This appears to be a 7% iOS JetStream regression. Can we roll it out?
> 
> Yes, but we might want to consider keeping the regression if the regressed
> tests were previously benefiting from their GC work happening in between
> measurement iterations. It's not in the spirit of the test, or of WebKit, to
> hide GC work from benchmarks. 
I totally agree; and I also agree it's worth making this determination before re-committing if this turns out to be the reason. I'm going to roll out for now until we know for sure what's happening.

> 
> Do you know what tests are regressed?

crypto-aes, crypto-md5, crypto-sha1, date-format-tofte, date-format-xparb, n-body, regex-dna, tagcloud

[WebKit Commit Bot]

Re-opened since this is blocked by bug 171842

[Filip Pizlo]

I looked into this some more with ggaren.

Currently on iOS, we do a sync GC during the unmeasured "idle" time between iterations in the tests that regressed (crypto-aes, crypto-md5, crypto-sha1, date-format-tofte, date-format-xparb, n-body, regex-dna, tagcloud).  It's not a goal of JetStream to avoid measuring GC time.  We want to measure any GC cost incurred by running these tests.

By making the tests async, some of the cost of running those tests (on the order of milliseconds) ends up happening while JetStream is measuring time.

Therefore, this change did not make GC take longer.  It just changed how much of GC is visible to JetStream.

Assuming other tests are fine, I think we should reland this and take the hit on JetStream.  Separately, we should investigate how we can make JetStream better at always measuring all GC time, even when the browser is being sneaky and trying to hide it from us.

[Filip Pizlo]

Created attachment 309633 [details]
patch for relanding

[Filip Pizlo]

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