Bug 189558 – [JSC] Heap::reportExtraMemoryVisited shows contention if we have many JSString

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

[Yusuke Suzuki]

When running web-tooling-benchmark postcss test on Linux JSCOnly port, we get the following result in `perf report`.

  10.95%  AutomaticThread  libJavaScriptCore.so.1.0.0  [.] JSC::Heap::reportExtraMemoryVisited

This is because postcss produces bunch of JSString, which requires reportExtraMemoryVisited calls in visitChildren.
And since reportExtraMemoryVisited attempts to update atomic counter, if we have bunch of marking threads, it just becomes contended!

[Saam Barati]

Nice find. Seems like we can update that shared value less frequently.

[Yusuke Suzuki]

Created attachment 350197 [details]
Patch

[Mark Lam]

Comment on attachment 350197 [details]
Patch

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

r=me

> Source/JavaScriptCore/heap/SlotVisitorInlines.h:162
> +        // FIXME: Change this to use SaturatedArithmetic when available.
> +        // https://bugs.webkit.org/show_bug.cgi?id=170411
> +        Checked<size_t, RecordOverflow> checkedNewSize = m_extraMemorySize;
> +        checkedNewSize += size;
> +        m_extraMemorySize = UNLIKELY(checkedNewSize.hasOverflowed()) ? std::numeric_limits<size_t>::max() : checkedNewSize.unsafeGet();

I suggest just making m_extraMemorySize a Checked<size_t, RecordOverflow> instead to simplify this.  That way, we won't be checking hasOverflowed() repeatedly.  Note: the Checked add operator would already have checked hasOverflowed().  Only need to check overflow in propagateExternalMemoryVisitedIfNecessary().

[Yusuke Suzuki]

Committed r236296: <https://trac.webkit.org/changeset/236296>

[Radar WebKit Bug Importer]

<rdar://problem/44663957>

[Yusuke Suzuki]

Comment on attachment 350197 [details]
Patch

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

>> Source/JavaScriptCore/heap/SlotVisitorInlines.h:162
>> +        m_extraMemorySize = UNLIKELY(checkedNewSize.hasOverflowed()) ? std::numeric_limits<size_t>::max() : checkedNewSize.unsafeGet();
> 
> I suggest just making m_extraMemorySize a Checked<size_t, RecordOverflow> instead to simplify this.  That way, we won't be checking hasOverflowed() repeatedly.  Note: the Checked add operator would already have checked hasOverflowed().  Only need to check overflow in propagateExternalMemoryVisitedIfNecessary().

Sounds nice. Fixed.

[Dawei Fenton (:realdawei)]

(In reply to Yusuke Suzuki from comment #4)
> Committed r236296: <https://trac.webkit.org/changeset/236296>

seeing crashes on the Sierra performance bot after this revision.

sample output: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Perf%29/builds/1661/steps/perf-test/logs/stdio
Running Speedometer (184 of 185)
crash: Speedometer
FAILED
Finished: 8.206435 s


crash log:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010e6ac900 JSC::JSRopeString::resolveRope(JSC::ExecState*) const + 496 (JSCellInlines.h:144)
1   com.apple.JavaScriptCore      	0x000000010e64add4 JSC::getCalculatedDisplayName(JSC::VM&, JSC::JSObject*) + 372 (JSFunction.cpp:651)
2   com.apple.JavaScriptCore      	0x000000010e3e6f36 JSC::StackVisitor::Frame::functionName() const + 134 (StackVisitor.cpp:298)
3   com.apple.JavaScriptCore      	0x000000010e3c9d92 Inspector::CreateScriptCallStackFunctor::operator()(JSC::StackVisitor&) const + 82 (ScriptCallStackFactory.cpp:71)
4   com.apple.JavaScriptCore      	0x000000010e3c43fb void JSC::ExecState::iterate<(JSC::StackVisitor::EmptyEntryFrameAction)0, Inspector::CreateScriptCallStackFunctor>(Inspector::CreateScriptCallStackFunctor const&) + 107 (CallFrame.h:284)
5   com.apple.JavaScriptCore      	0x000000010e3c44c2 Inspector::createScriptCallStackForConsole(JSC::ExecState*, unsigned long) + 114 (ScriptCallStackFactory.cpp:117)
6   com.apple.JavaScriptCore      	0x000000010e362b1f Inspector::ConsoleMessage::autogenerateMetadata(JSC::ExecState*) + 47 (RefPtr.h:181)
7   com.apple.WebCore             	0x000000010b43e44d WebCore::PageConsoleClient::messageWithTypeAndLevel(JSC::MessageType, JSC::MessageLevel, JSC::ExecState*, WTF::Ref<Inspector::ScriptArguments, WTF::DumbPtrTraits<Inspector::ScriptArguments> >&&) + 125 (PageConsoleClient.cpp:168)
8   com.apple.JavaScriptCore      	0x000000010e5eb4f7 JSC::consoleLogWithLevel(JSC::ExecState*, JSC::MessageLevel) + 135 (ConsoleObject.cpp:117)
9   ???                           	0x0000045eac32c177 0 + 4804662444407
10  ???                           	0x0000045eac60f99c 0 + 4804665473436
11  com.apple.JavaScriptCore      	0x000000010dea5a04 llint_entry + 26817
12  com.apple.JavaScriptCore      	0x000000010dea5a04 llint_entry + 26817
13  com.apple.JavaScriptCore      	0x000000010dea5a04 llint_entry + 26817
14  com.apple.JavaScriptCore      	0x000000010dea5d97 llint_entry + 27732
15  com.apple.JavaScriptCore      	0x000000010dea5a04 llint_entry + 26817
16  com.apple.JavaScriptCore      	0x000000010dea5cfa llint_entry + 27575
17  com.apple.JavaScriptCore      	0x000000010dea5a04 llint_entry + 26817
18  com.apple.JavaScriptCore      	0x000000010de9ef89 vmEntryToJavaScript + 200
19  com.apple.JavaScriptCore      	0x000000010e3e328a JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) + 11146 (Interpreter.cpp:834)
20  com.apple.JavaScriptCore      	0x000000010e5e4f23 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 307 (Completion.cpp:103)
21  com.apple.WebCore             	0x000000010ade3084 WebCore::JSExecState::profiledEvaluate(JSC::ExecState*, JSC::ProfilingReason, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 84 (JSExecState.h:80)
22  com.apple.WebCore             	0x000000010ade2f08 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) + 184 (ScriptController.cpp:131)
23  com.apple.WebCore             	0x000000010b05614a WebCore::ScriptElement::executeClassicScript(WebCore::ScriptSourceCode const&) + 538 (ScriptElement.cpp:388)
24  com.apple.WebCore             	0x000000010b0264a3 WebCore::LoadableClassicScript::execute(WebCore::ScriptElement&) + 147 (LoadableClassicScript.cpp:123)
25  com.apple.WebCore             	0x000000010b0563bd WebCore::ScriptElement::executeScriptAndDispatchEvent(WebCore::LoadableScript&) + 173 (ScriptElement.cpp:427)
26  com.apple.WebCore             	0x000000010b26a16b WebCore::HTMLScriptRunner::executeParsingBlockingScripts() + 203 (HTMLScriptRunner.cpp:164)
27  com.apple.WebCore             	0x000000010b2602fc WebCore::HTMLDocumentParser::notifyFinished(WebCore::PendingScript&) + 60 (HTMLDocumentParser.cpp:564)
28  com.apple.WebCore             	0x000000010b040a23 WebCore::PendingScript::notifyFinished(WebCore::LoadableScript&) + 35 (RefCounted.h:144)
29  com.apple.WebCore             	0x000000010b02639c WebCore::LoadableScript::notifyClientFinished() + 300 (LoadableScript.cpp:59)
30  com.apple.WebCore             	0x000000010b026256 WebCore::LoadableClassicScript::notifyFinished(WebCore::CachedResource&) + 1238 (LoadableClassicScript.cpp:118)
31  com.apple.WebCore             	0x000000010b3b4153 WebCore::CachedResource::checkNotify() + 195 (Vector.h:686)
32  com.apple.WebCore             	0x000000010b385eb0 WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&) + 1168 (SubresourceLoader.cpp:636)
33  com.apple.WebKit              	0x00000001082067b2 WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&) + 220 (WebResourceLoader.cpp:162)
34  com.apple.WebKit              	0x00000001082e23b7 void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)) + 137 (HandleMessage.h:134)
35  com.apple.WebKit              	0x0000000108200739 WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 777 (NetworkProcessConnection.cpp:120)
36  com.apple.WebKit              	0x0000000107ed4546 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 108 (Connection.cpp:986)
37  com.apple.WebKit              	0x0000000107ed7afd IPC::Connection::dispatchOneIncomingMessage() + 179 (Connection.cpp:1053)
38  com.apple.JavaScriptCore      	0x000000010dcd652f WTF::RunLoop::performWork() + 447 (RunLoop.cpp:123)
39  com.apple.JavaScriptCore      	0x000000010dcd6762 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
40  com.apple.CoreFoundation      	0x00007fff988e93e1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
41  com.apple.CoreFoundation      	0x00007fff988ca65c __CFRunLoopDoSources0 + 556
42  com.apple.CoreFoundation      	0x00007fff988c9b46 __CFRunLoopRun + 934
43  com.apple.CoreFoundation      	0x00007fff988c9544 CFRunLoopRunSpecific + 420
44  com.apple.HIToolbox           	0x00007fff97e28ebc RunCurrentEventLoopInMode + 240
45  com.apple.HIToolbox           	0x00007fff97e28cf1 ReceiveNextEventCommon + 432
46  com.apple.HIToolbox           	0x00007fff97e28b26 _BlockUntilNextEventMatchingListInModeWithFilter + 71
47  com.apple.AppKit              	0x00007fff963bfa54 _DPSNextEvent + 1120
48  com.apple.AppKit              	0x00007fff96b3b7ee -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 2796
49  com.apple.AppKit              	0x00007fff963b43db -[NSApplication run] + 926
50  com.apple.AppKit              	0x00007fff9637ee0e NSApplicationMain + 1237
51  libxpc.dylib                  	0x00007fffae6e28c7 _xpc_objc_main + 775
52  libxpc.dylib                  	0x00007fffae6e12e4 xpc_main + 494
53  com.apple.WebKit.WebContent   	0x0000000107eba68d WebKit::XPCServiceMain(int, char const**) + 490 (XPCServiceMain.mm:123)
54  com.apple.WebKit.WebContent   	0x0000000107eba81b main + 9 (XPCServiceMain.mm:46)
55  libdyld.dylib                 	0x000

[Mark Lam]

(In reply to Dawei Fenton (:realdawei) from comment #7)
> (In reply to Yusuke Suzuki from comment #4)
> > Committed r236296: <https://trac.webkit.org/changeset/236296>
> 
> seeing crashes on the Sierra performance bot after this revision.

I'm fixing this in https://bugs.webkit.org/show_bug.cgi?id=189855.

[Dawei Fenton (:realdawei)]

(In reply to Mark Lam from comment #8)
> (In reply to Dawei Fenton (:realdawei) from comment #7)
> > (In reply to Yusuke Suzuki from comment #4)
> > > Committed r236296: <https://trac.webkit.org/changeset/236296>
> > 
> > seeing crashes on the Sierra performance bot after this revision.
> 
> I'm fixing this in https://bugs.webkit.org/show_bug.cgi?id=189855.

great thanks!

[Alexey Proskuryakov]

Sounds like this hasn't been rolled back, so marking as resolved again.