Summary: | Structures should be allocated out of an aligned pool of memory so StructureID->Structure* is fast. | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||||||||||||||||||||||||||||||||
Status: | REOPENED --- | ||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, benjamin, cdumez, cmarcelo, ews-watchlist, gyuyoung.kim, Hironori.Fujii, mark.lam, mcatanzaro, msaboff, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=233720 https://bugs.webkit.org/show_bug.cgi?id=235720 |
||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Keith Miller
2021-11-19 12:56:25 PST
Created attachment 444852 [details]
Patch
Created attachment 444863 [details]
Patch
Comment on attachment 444863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444863&action=review > Source/JavaScriptCore/runtime/JSCConfig.h:43 > +constexpr uintptr_t structureHeapAddressSize = 256 * MB; In old days, our # of StructureIDs are https://trac.webkit.org/changeset/281319/webkit => 1 << 24. And since each Structure has 112 bytes, maximum size was 1879048192, 1792MB. And it was too small and we extended it to 1 << 26, which is 7168MB. So, 256MB is too small. Maybe, 2 or 4GB is better? Created attachment 444962 [details]
Patch
Created attachment 444986 [details]
Patch
Comment on attachment 444986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444986&action=review r=me with 32bit and WIndows fixes. And please monitor performance / schedule A-B test of (Speedometer2, JetStream2) / memory change after landing to ensure that this does not cause the regression. > Source/JavaScriptCore/heap/StructureAllignedMemoryAllocator.h:1 > +/* This file is duplicate to Struc tureAlignedMemoryAllocator.h This is mis-typed "Alligned". Created attachment 445142 [details]
Patch for landing
Created attachment 445143 [details]
Patch for landing
Created attachment 445145 [details]
Patch
Created attachment 445146 [details]
Patch
Created attachment 445147 [details]
Patch
Created attachment 445150 [details]
Patch
Created attachment 445204 [details]
Patch
WTF.dll!abort() Line 77 C++ WTF.dll!WTF::OSAllocator::releaseDecommitted(void * address, unsigned __int64 bytes) Line 91 C++ JavaScriptCore.dll!JSC::StructureAlignedMemoryAllocator::initializeStructureAddressSpace() Line 140 C++ [Inline Frame] JavaScriptCore.dll!JSC::initialize::__l2::<lambda_1>::operator()() Line 69 C++ [Inline Frame] JavaScriptCore.dll!std::invoke(JSC::initialize::__l2::<lambda_1> &&) Line 1585 C++ [Inline Frame] JavaScriptCore.dll!std::call_once(std::once_flag &) Line 568 C++ JavaScriptCore.dll!JSC::initialize() Line 58 C++ WebKit.dll!WebKitClassFactory::WebKitClassFactory(_GUID targetClass) Line 68 C++ WebKit.dll!DllGetClassObject(const _GUID & rclsid, const _GUID & riid, void * * ppv) Line 122 C++ [Inline Frame] WebKit.dll!classFactory(const _GUID &) Line 61 C++ WebKit.dll!WebKitCreateInstance(const _GUID & rclsid, IUnknown * pUnkOuter, const _GUID & riid, void * * ppvObject) Line 72 C++ MiniBrowserLib.dll!WebKitLegacyBrowserWindow::init() Line 79 C++ MiniBrowserLib.dll!MainWindow::init(std::function<WTF::Ref<BrowserWindow,WTF::RawPtrTraits<BrowserWindow>> __cdecl(BrowserWindowClient &,HWND__ *,bool)> factory, HINSTANCE__ * hInstance, bool usesLayeredWebView) Line 226 C++ MiniBrowserLib.dll!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 93 C++ MiniBrowser.exe!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 225 C++ [Inline Frame] MiniBrowser.exe!invoke_main() Line 118 C++ MiniBrowser.exe!__scrt_common_main_seh() Line 288 C++ kernel32.dll!00007ffd87f67034() Unknown ntdll.dll!00007ffd89442651() Unknown Created attachment 445431 [details]
Patch
Created attachment 445436 [details]
Patch
This patch can't compile for Windows Debug builds. AppleWin and WinCairo don't have Debug build EWS bots.
> Source\WTF\wtf\win\OSAllocatorWin.cpp(52): error C3861: 'pageSize': identifier not found
Created attachment 445466 [details]
layout-test-results (WinCairo WK1 Debug)
python3.exe ./Tools/Scripts/run-webkit-tests --wincairo --debug --no-new-test-results --no-retry-failures -1 --test-list=test.txt
ASSERTION FAILED: reinterpret_cast<void*>(roundUpToMultipleOf(bytes, reinterpret_cast<uintptr_t>(result))) == result
C:\home\webkit\gc\Source\WTF\wtf\win\OSAllocatorWin.cpp(54) : WTF::OSAllocator::reserveUncommittedAligned
1 00007FFD470A217B WTFCrash
2 00007FFD470A52ED WTFCrashWithInfo
3 00007FFD4721C91E WTF::OSAllocator::reserveUncommittedAligned
4 00007FFD2A0FBAD6 JSC::StructureAlignedMemoryAllocator::initializeStructureAddressSpace
5 00007FFD2A785AFE `JSC::initialize'::`2'::<lambda_1>::operator()
6 00007FFD2A79BAC4 std::invoke<`JSC::initialize'::`2'::<lambda_1> >
7 00007FFD2A798ED0 std::call_once<`JSC::initialize'::`2'::<lambda_1> >
8 00007FFD2A785A98 JSC::initialize
9 00007FFCF906425F WebKitClassFactory::WebKitClassFactory
10 00007FFCF90651D4 DllGetClassObject
11 00007FFCF9061513 classFactory
12 00007FFCF9061311 WebKitCreateInstance
13 00007FFD3917B355 initialize
14 00007FFD39182096 main
15 00007FFD3918274C dllLauncherEntryPoint
16 00007FF648161A0C main
17 00007FF648165494 __scrt_common_main_seh
18 00007FFD87F67034 BaseThreadInitThunk
19 00007FFD89442651 RtlUserThreadStart
Created attachment 445487 [details]
Patch for landing
ChangeLog entry in Source/WTF/ChangeLog contains OOPS!. Created attachment 445488 [details]
Patch for landing
This patch can't compile. C:\home\webkit\gc\Source\WTF\wtf\win\OSAllocatorWin.cpp(52): error C3861: 'pageSize': identifier not found C:\home\webkit\gc\Source\WTF\wtf\win\OSAllocatorWin.cpp(56): error C2065: 'mapped': undeclared identifier Created attachment 445493 [details]
Patch for landing
Comment on attachment 445493 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=445493&action=review > Source/WTF/wtf/win/OSAllocatorWin.cpp:54 > + // FIXME: Is there a way to do this where we can either release the excess reservation or not reserve it at all? Filed: Bug 233674 – [Win] OSAllocator::reserveUncommittedAligned should use aligned allocation Committed r286345 (244704@main): <https://commits.webkit.org/244704@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445493 [details]. Comment on attachment 445493 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=445493&action=review > Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:91 > + MarkedBlock* block = reinterpret_cast<MarkedBlock*>(g_jscConfig.startOfStructureHeap) + freeIndex * MarkedBlock::blockSize; Why “* markedBlockSize”? Doesn’t pointer arithmetic already take that into account? Or maybe do arithmetic on char*? This patch broke cloop on Linux, there are lots of crashes in JSC::LLInt::CLoop::execute: #0 0x00007f42933f159f in JSC::LLInt::CLoop::execute(JSC::OpcodeID, void*, JSC::VM*, JSC::ProtoCallFrame*, bool) () from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libJavaScriptCore.so.1 #1 0x00007f429369b78a in vmEntryToJavaScript () from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libJavaScriptCore.so.1 #2 0x00007f429366cf9f in JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) () from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libJavaScriptCore.so.1 #3 0x00007f42939c4956 in JSC::Walker::callReviver(JSC::JSObject*, JSC::JSValue, JSC::JSValue) () from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libJavaScriptCore.so.1 #4 0x00007f42939bdac3 in JSC::Walker::walk(JSC::JSValue) () from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libJavaScriptCore.so.1 #5 0x00007f42939be3cd in JSC::jsonProtoFuncParse(JSC::JSGlobalObject*, JSC::CallFrame*) () from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libJavaScriptCore.so.1 #6 0x00007f42933ea0c8 in JSC::LLInt::CLoop::execute(JSC::OpcodeID, void*, JSC::VM*, JSC::ProtoCallFrame*, bool) () from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libJavaScriptCore.so.1 #7 0x00007f429369b78a in vmEntryToJavaScript () from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libJavaScriptCore.so.1 #8 0x00007f429366a080 in JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::JSGlobalObject*, JSC::JSObject*) () from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libJavaScriptCore.so.1 #9 0x00007f4293848dc1 in JSC::evaluate(JSC::JSGlobalObject*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) () from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libJavaScriptCore.so.1 #10 0x00000000004168ac in jscmain(int, char**) () #11 0x000000000040a1ab in main () I'm going to see if I can get a better backtrace. Crash is here: Program terminated with signal SIGSEGV, Segmentation fault. #0 JSC::LLInt::CLoop::execute (entryOpcodeID=JSC::llint_vm_entry_to_javascript, executableAddress=0x7f93e5fba23d <JSC::LLInt::CLoop::execute(JSC::OpcodeID, void*, JSC::VM*, JSC::ProtoCallFrame*, bool)+44059>, vm=0x168ae30, protoCallFrame=0x7fff3b91abe0, isInitializationPass=false) at /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/LLIntAssembly.h:8918 8918 t2 = *CAST<int64_t*>(t2.i8p() + 32); // LowLevelInterpreter64.asm:1670 I will attach a full backtrace, but it's pretty intense because each frame is huge. Created attachment 445583 [details]
Full backtrace
(In reply to Michael Catanzaro from comment #28) > This patch broke cloop on Linux, there are lots of crashes in > JSC::LLInt::CLoop::execute: I checked first with USE_SYSTEM_MALLOC=ON, but it's also broken without. Comment on attachment 445493 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=445493&action=review >> Source/JavaScriptCore/heap/StructureAlignedMemoryAllocator.cpp:91 >> + MarkedBlock* block = reinterpret_cast<MarkedBlock*>(g_jscConfig.startOfStructureHeap) + freeIndex * MarkedBlock::blockSize; > > Why “* markedBlockSize”? Doesn’t pointer arithmetic already take that into account? Or maybe do arithmetic on char*? I think sizeof(MarkedBlock) == 1 since it does not have any fields actually. But I think this looks misleading. bitwise_cast<MarkedBlock*>(g_jscConfig.startOfStructureHeap + freeIndex * MarkedBlock::blockSize) is better. Committed r286372 (244731@main): <https://commits.webkit.org/244731@main> (In reply to Yusuke Suzuki from comment #33) > Committed r286372 (244731@main): <https://commits.webkit.org/244731@main> Confirmed fixed, thanks Yusuke! One more thing. Previously StructureID was just a uint32_t, but now it's a struct with a nontrivial constructor. It introduced a GCC warning: [943/1704] Building CXX object Source/JavaScriptCore/CMake...vedSources/unified-sources/UnifiedSource-f2e18ffc-36.cpp.o In file included from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f2e18ffc-36.cpp:3: /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/StructureChain.cpp: In static member function ‘static JSC::StructureChain* JSC::StructureChain::create(JSC::VM&, JSC::JSObject*)’: /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/StructureChain.cpp:52:11: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘class JSC::StructureID’; use assignment or value-initialization instead [-Wclass-memaccess] 52 | memset(vector, 0, bytes); | ~~~~~~^~~~~~~~~~~~~~~~~~ In file included from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/Heap.h:46, from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/JSCell.h:29, from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/JSCast.h:28, from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/ClassInfo.h:26, from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/Structure.h:28, from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/Structure.cpp:28, from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f2e18ffc-36.cpp:1: /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/StructureID.h:37:7: note: ‘class JSC::StructureID’ declared here 37 | class StructureID { | ^~~~~~~~~~~ This is one of the rare cases where GCC is better at warning than Clang. Anyway, if I'm reading the code correctly, I think this memset() can be safely deleted, yes? It was previously required to initialize the vector of StructureIDs, but now that StructureID is a struct, that's handled by its constructor. (In reply to Michael Catanzaro from comment #35) > One more thing. Previously StructureID was just a uint32_t, but now it's a > struct with a nontrivial constructor. It introduced a GCC warning: > > [943/1704] Building CXX object > Source/JavaScriptCore/CMake...vedSources/unified-sources/UnifiedSource- > f2e18ffc-36.cpp.o > In file included from > /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/ > DerivedSources/unified-sources/UnifiedSource-f2e18ffc-36.cpp:3: > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/ > StructureChain.cpp: In static member function ‘static JSC::StructureChain* > JSC::StructureChain::create(JSC::VM&, JSC::JSObject*)’: > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/ > StructureChain.cpp:52:11: warning: ‘void* memset(void*, int, size_t)’ > clearing an object of non-trivial type ‘class JSC::StructureID’; use > assignment or value-initialization instead [-Wclass-memaccess] > 52 | memset(vector, 0, bytes); > | ~~~~~~^~~~~~~~~~~~~~~~~~ > In file included from > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/heap/Heap.h:46, > from > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/JSCell.h:29, > from > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/JSCast.h:28, > from > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/ClassInfo.h: > 26, > from > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/Structure.h: > 28, > from > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/Structure.cpp: > 28, > from > /home/mcatanzaro/Projects/WebKit/WebKitBuild/Debug/JavaScriptCore/ > DerivedSources/unified-sources/UnifiedSource-f2e18ffc-36.cpp:1: > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/runtime/StructureID.h: > 37:7: note: ‘class JSC::StructureID’ declared here > 37 | class StructureID { > | ^~~~~~~~~~~ > > This is one of the rare cases where GCC is better at warning than Clang. > Anyway, if I'm reading the code correctly, I think this memset() can be > safely deleted, yes? It was previously required to initialize the vector of > StructureIDs, but now that StructureID is a struct, that's handled by its > constructor. No the memset is needed since we don't initialize the memory of that buffer without it. (In reply to Keith Miller from comment #36) > No the memset is needed since we don't initialize the memory of that buffer > without it. Ah, right... I think in this case we can just suppress the warning wicth static_cast<void*>(vector), but that's fragile because if the StructureID constructor starts doing something else in the future, it will be broken and we won't get another warning. So it would be better to rewrite this IMO. I think void* is the best fix. It is extremely unlikely that StructureID constructor does something meaningful. Committed r286382 (244741@main): <https://commits.webkit.org/244741@main> Will roll out in https://bugs.webkit.org/show_bug.cgi?id=234268 |