Bug 233379 – Structures should be allocated out of an aligned pool of memory so StructureID->Structure* is fast.

• Status: REOPENED
• Component: New Bugs

[Keith Miller]

Structures should be allocated out of an aligned pool of memory so StructureID->Structure* is fast.

[Keith Miller]

Created attachment 444852 [details]
Patch

[Keith Miller]

Created attachment 444863 [details]
Patch

[Yusuke Suzuki]

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?

[Keith Miller]

Created attachment 444962 [details]
Patch

[Keith Miller]

Created attachment 444986 [details]
Patch

[Yusuke Suzuki]

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

[Keith Miller]

Created attachment 445142 [details]
Patch for landing

[Keith Miller]

Created attachment 445143 [details]
Patch for landing

[Keith Miller]

Created attachment 445145 [details]
Patch

[Keith Miller]

Created attachment 445146 [details]
Patch

[Keith Miller]

Created attachment 445147 [details]
Patch

[Keith Miller]

Created attachment 445150 [details]
Patch

[Keith Miller]

Created attachment 445204 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85771775>

[Fujii Hironori]

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

[Keith Miller]

Created attachment 445431 [details]
Patch

[Keith Miller]

Created attachment 445436 [details]
Patch

[Fujii Hironori]

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

[Fujii Hironori]

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

[Keith Miller]

Created attachment 445487 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

[Keith Miller]

Created attachment 445488 [details]
Patch for landing

[Fujii Hironori]

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

[Keith Miller]

Created attachment 445493 [details]
Patch for landing

[Fujii Hironori]

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

[EWS]

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

[Saam Barati]

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

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 445583 [details]
Full backtrace

[Michael Catanzaro]

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

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r286372 (244731@main): <https://commits.webkit.org/244731@main>

[Michael Catanzaro]

(In reply to Yusuke Suzuki from comment #33)
> Committed r286372 (244731@main): <https://commits.webkit.org/244731@main>

Confirmed fixed, thanks Yusuke!

[Michael Catanzaro]

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.

[Keith Miller]

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

[Michael Catanzaro]

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

[Yusuke Suzuki]

I think void* is the best fix. It is extremely unlikely that StructureID constructor does something meaningful.

[Yusuke Suzuki]

Committed r286382 (244741@main): <https://commits.webkit.org/244741@main>

[Saam Barati]

Will roll out in https://bugs.webkit.org/show_bug.cgi?id=234268