Bug 171708

Created attachment 309128 [details] the patch

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.

(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'll wait until tomorrow to land this. I want the bots to see it separately from the previous change I just landed.

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

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

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

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

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**)

Created attachment 309173 [details] patch for landing Try EWS again.

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

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

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

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

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

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

(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

Re-opened since this is blocked by bug 171842

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.

Created attachment 309633 [details] patch for relanding

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