Bug 202105 – Reduce the amount of memory needed to store Options.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Details to come in ChangeLog.

[Mark Lam]

Created attachment 379371 [details]
proposed patch.

Let's get some EWS testing.

[Mark Lam]

Created attachment 379372 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 379372 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/runtime/JSCConfig.h:42
>  #if CPU(ARM64) || PLATFORM(WATCHOS)
>  constexpr size_t PageSize = 16 * KB;
> +constexpr size_t ConfigSizeToProtect = 1 * PageSize;
>  #else
>  constexpr size_t PageSize = 4 * KB;
> +constexpr size_t ConfigSizeToProtect = 1 * PageSize;
>  #endif

Add `PageSize = 64 * KB` for `CPU(PPC64) || CPU(PPC64LE) || CPU(PPC) || CPU(UNKNOWN)`.

[Mark Lam]

Thanks for the review.

(In reply to Yusuke Suzuki from comment #3)
> > Source/JavaScriptCore/runtime/JSCConfig.h:42
> >  #if CPU(ARM64) || PLATFORM(WATCHOS)
> >  constexpr size_t PageSize = 16 * KB;
> > +constexpr size_t ConfigSizeToProtect = 1 * PageSize;
> >  #else
> >  constexpr size_t PageSize = 4 * KB;
> > +constexpr size_t ConfigSizeToProtect = 1 * PageSize;
> >  #endif
> 
> Add `PageSize = 64 * KB` for `CPU(PPC64) || CPU(PPC64LE) || CPU(PPC) ||
> CPU(UNKNOWN)`.

I've added this case.

Landed in r250262: <http://trac.webkit.org/r250262>.

[Radar WebKit Bug Importer]

<rdar://problem/55641041>

[Fujii Hironori]

Windows port can't compile:

https://build.webkit.org/builders/Apple%20Win%2010%20Release%20%28Build%29/builds/6980

> c:\cygwin\worker\win10-release\build\webkitbuild\release\derivedsources\forwardingheaders\javascriptcore\optionslist.h(590): fatal error C1060: compiler is out of heap space (compiling source file C:\cygwin\worker\win10-release\build\WebKitBuild\Release\DerivedSources\WebCore\JSInternals.cpp) [C:\cygwin\worker\win10-release\build\WebKitBuild\Release\Source\WebCore\WebCoreTestSupport.vcxproj]
> c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.15.26726\include\utility(405): fatal error C1060: compiler is out of heap space (compiling source file C:\cygwin\worker\win10-release\build\Source\WebKitLegacy\win\Plugins\PluginPackage.cpp) [C:\cygwin\worker\win10-release\build\WebKitBuild\Release\Source\WebKitLegacy\WebKitLegacy.vcxproj]

[Konstantin Tokarev]

>fatal error C1060: compiler is out of heap space

I guess it uses x86_amd64 toolchain (i.e., 32-bit compiler and other tools) instead of amd64

[Fujii Hironori]

Yes, AppleWin has only 32bit Buildbots.

However, WinCairo support only 64bit. And, its buildbots are reporting same errors:

WinCairo 64-bit WKL Debug (Build)
WinCairo 64-bit WKL Release (Build)

https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Build%29/builds/12188

> FAILED: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f2e18ffc-26.cpp.obj 
> C:\MSVS\VC\Tools\MSVC\14.20.27508\bin\Hostx64\x64\cl.exe  /nologo /TP -DBUILDING_JavaScriptCore -DBUILDING_WITH_CMAKE=1 -DHAVE_CONFIG_H=1 -DJavaScriptCore_EXPORTS -DNOCRYPT -DNOMINMAX -DUNICODE -DWINVER=0x601 -DWTF_PLATFORM_WIN_CAIRO=1 -D_CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES=1 -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -D_WIN32_WINNT=0x601 -D_WINDOWS -D_WINSOCKAPI_="" -IJavaScriptCore\Headers -I. -I..\..\Source\JavaScriptCore -I..\..\Source\JavaScriptCore\API -I..\..\Source\JavaScriptCore\assembler -I..\..\Source\JavaScriptCore\b3 -I..\..\Source\JavaScriptCore\b3\air -I..\..\Source\JavaScriptCore\bindings -I..\..\Source\JavaScriptCore\builtins -I..\..\Source\JavaScriptCore\bytecode -I..\..\Source\JavaScriptCore\bytecompiler -I..\..\Source\JavaScriptCore\dfg -I..\..\Source\JavaScriptCore\disassembler -I..\..\Source\JavaScriptCore\disassembler\ARM64 -I..\..\Source\JavaScriptCore\disassembler\udis86 -I..\..\Source\JavaScriptCore\domjit -I..\..\Source\JavaScriptCore\ftl -I..\..\Source\JavaScriptCore\heap -I..\..\Source\JavaScriptCore\debugger -I..\..\Source\JavaScriptCore\inspector -I..\..\Source\JavaScriptCore\inspector\agents -I..\..\Source\JavaScriptCore\inspector\augmentable -I..\..\Source\JavaScriptCore\inspector\remote -I..\..\Source\JavaScriptCore\interpreter -I..\..\Source\JavaScriptCore\jit -I..\..\Source\JavaScriptCore\llint -I..\..\Source\JavaScriptCore\parser -I..\..\Source\JavaScriptCore\profiler -I..\..\Source\JavaScriptCore\runtime -I..\..\Source\JavaScriptCore\tools -I..\..\Source\JavaScriptCore\wasm -I..\..\Source\JavaScriptCore\wasm\js -I..\..\Source\JavaScriptCore\yarr -IJavaScriptCore\DerivedSources -IJavaScriptCore\DerivedSources\inspector -IJavaScriptCore\DerivedSources\runtime -IJavaScriptCore\DerivedSources\yarr -I..\include\private -I..\..\Source\JavaScriptCore\inspector\remote\socket -IWTF\Headers -IDerivedSources -I..\..\Source\ThirdParty -I..\..\WebKitLibraries\win\include /W4 /DWIN32 /D_WINDOWS  /GR- /EHs- /EHc-  /MD /O2 /Ob2 /DNDEBUG   /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180 /wd4189 /wd4201 /wd4206 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291 /wd4305 /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458 /wd4459 /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611 /wd4646 /wd4702 /wd4706 /wd4722 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996 /wd6011 /wd6031 /wd6211 /wd6246 /wd6255 /wd6387 /Zi /GS /EHa- /EHc- /EHs- /fp:except- /analyze- /bigobj /utf-8 /validate-charset /Oy- -std:c++17 /showIncludes /FoSource\JavaScriptCore\CMakeFiles\JavaScriptCore.dir\__\__\JavaScriptCore\DerivedSources\unified-sources\UnifiedSource-f2e18ffc-26.cpp.obj /FdSource\JavaScriptCore\CMakeFiles\JavaScriptCore.dir\ /FS -c JavaScriptCore\DerivedSources\unified-sources\UnifiedSource-f2e18ffc-26.cpp
> C:\WebKit-BuildWorker\wincairo-wkl-release\build\Source\JavaScriptCore\runtime\PropertyMapHashTable.h(289) : fatal error C1002: compiler is out of heap space in pass 2
> C:\WebKit-BuildWorker\wincairo-wkl-release\build\Source\JavaScriptCore\runtime\Uint16WithFraction.h(140) : fatal error C1002: compiler is out of heap space in pass 2


https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Debug%20%28Build%29/builds/10577

> FAILED: Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f2e18ffc-31.cpp.obj 
> C:\MSVS\VC\Tools\MSVC\14.21.27702\bin\Hostx64\x64\cl.exe  /nologo /TP -DBUILDING_JavaScriptCore -DBUILDING_WITH_CMAKE=1 -DHAVE_CONFIG_H=1 -DJavaScriptCore_EXPORTS -DNOCRYPT -DNOMINMAX -DUNICODE -DWINVER=0x601 -DWTF_PLATFORM_WIN_CAIRO=1 -D_CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES=1 -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_UNICODE -D_WIN32_WINNT=0x601 -D_WINDOWS -D_WINSOCKAPI_="" -IJavaScriptCore\Headers -I. -I..\..\Source\JavaScriptCore -I..\..\Source\JavaScriptCore\API -I..\..\Source\JavaScriptCore\assembler -I..\..\Source\JavaScriptCore\b3 -I..\..\Source\JavaScriptCore\b3\air -I..\..\Source\JavaScriptCore\bindings -I..\..\Source\JavaScriptCore\builtins -I..\..\Source\JavaScriptCore\bytecode -I..\..\Source\JavaScriptCore\bytecompiler -I..\..\Source\JavaScriptCore\dfg -I..\..\Source\JavaScriptCore\disassembler -I..\..\Source\JavaScriptCore\disassembler\ARM64 -I..\..\Source\JavaScriptCore\disassembler\udis86 -I..\..\Source\JavaScriptCore\domjit -I..\..\Source\JavaScriptCore\ftl -I..\..\Source\JavaScriptCore\heap -I..\..\Source\JavaScriptCore\debugger -I..\..\Source\JavaScriptCore\inspector -I..\..\Source\JavaScriptCore\inspector\agents -I..\..\Source\JavaScriptCore\inspector\augmentable -I..\..\Source\JavaScriptCore\inspector\remote -I..\..\Source\JavaScriptCore\interpreter -I..\..\Source\JavaScriptCore\jit -I..\..\Source\JavaScriptCore\llint -I..\..\Source\JavaScriptCore\parser -I..\..\Source\JavaScriptCore\profiler -I..\..\Source\JavaScriptCore\runtime -I..\..\Source\JavaScriptCore\tools -I..\..\Source\JavaScriptCore\wasm -I..\..\Source\JavaScriptCore\wasm\js -I..\..\Source\JavaScriptCore\yarr -IJavaScriptCore\DerivedSources -IJavaScriptCore\DerivedSources\inspector -IJavaScriptCore\DerivedSources\runtime -IJavaScriptCore\DerivedSources\yarr -I..\include\private -I..\..\Source\JavaScriptCore\inspector\remote\socket -IWTF\Headers -IDerivedSources -I..\..\Source\ThirdParty -I..\..\WebKitLibraries\win\include /W4 /DWIN32 /D_WINDOWS  /GR- /EHs- /EHc-  /MD /Zi /Ob0 /Od /RTC1   /wd4018 /wd4068 /wd4099 /wd4100 /wd4127 /wd4138 /wd4146 /wd4180 /wd4189 /wd4201 /wd4206 /wd4244 /wd4251 /wd4267 /wd4275 /wd4288 /wd4291 /wd4305 /wd4309 /wd4344 /wd4355 /wd4389 /wd4396 /wd4456 /wd4457 /wd4458 /wd4459 /wd4481 /wd4503 /wd4505 /wd4510 /wd4512 /wd4530 /wd4610 /wd4611 /wd4646 /wd4702 /wd4706 /wd4722 /wd4800 /wd4819 /wd4951 /wd4952 /wd4996 /wd6011 /wd6031 /wd6211 /wd6246 /wd6255 /wd6387 /Zi /GS /EHa- /EHc- /EHs- /fp:except- /analyze- /bigobj /utf-8 /validate-charset -std:c++17 /showIncludes /FoSource\JavaScriptCore\CMakeFiles\JavaScriptCore.dir\__\__\JavaScriptCore\DerivedSources\unified-sources\UnifiedSource-f2e18ffc-31.cpp.obj /FdSource\JavaScriptCore\CMakeFiles\JavaScriptCore.dir\ /FS -c JavaScriptCore\DerivedSources\unified-sources\UnifiedSource-f2e18ffc-31.cpp
> C:\WebKit-BuildWorker\wincairo-wkl-debug\build\WebKitBuild\Debug\WTF\Headers\wtf/SetForScope.h(43): fatal error C1060: compiler is out of heap space

[Konstantin Tokarev]

>Yes, AppleWin has only 32bit Buildbots.

They could switch to 64-bit toolchain to produce 32-bit binaries. It's probably called amd64_x86 in vcvarsall

[Mark Lam]

(In reply to Konstantin Tokarev from comment #9)
> >Yes, AppleWin has only 32bit Buildbots.
> 
> They could switch to 64-bit toolchain to produce 32-bit binaries. It's
> probably called amd64_x86 in vcvarsall

Looks like a MSVC bug:
https://developercommunity.visualstudio.com/content/problem/653301/fatal-error-c1002-compiler-is-out-of-heap-space-in.html

Posted on Sep 6: "A fix for this issue has been internally implemented and is being prepared for release. We’ll update you once it becomes available for download".

[Fujii Hironori]

(In reply to Konstantin Tokarev from comment #9)
> They could switch to 64-bit toolchain to produce 32-bit binaries. It's
> probably called amd64_x86 in vcvarsall

According the compilation log of comment 6, AppleWin is using 'HostX64\x86\CL.exe'.

>   INTERNAL COMPILER ERROR in 'C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.15.26726\bin\HostX64\x86\CL.exe'
>       Please choose the Technical Support command on the Visual C++
>       Help menu, or open the Technical Support help file for more information
> cl : Command line error D8040: error creating or communicating with child process [C:\cygwin\worker\win10-release\build\WebKitBuild\Release\Source\WebCore\WebCoreTestSupport.vcxproj]

[Fujii Hironori]

(In reply to Mark Lam from comment #10)
> Looks like a MSVC bug:
> https://developercommunity.visualstudio.com/content/problem/653301/fatal-
> error-c1002-compiler-is-out-of-heap-space-in.html

Nice to find it out.

> To workaround this issue, please use the x64 hosted toolset or use /cgthreads1 to disable multithreaded compilation.

It must be unacceptable for Windows devs to use /cgthreads1.
Let's revert the patch until the upcoming MSVC release.

[Mark Lam]

(In reply to Fujii Hironori from comment #12)
> (In reply to Mark Lam from comment #10)
> > Looks like a MSVC bug:
> > https://developercommunity.visualstudio.com/content/problem/653301/fatal-
> > error-c1002-compiler-is-out-of-heap-space-in.html
> 
> Nice to find it out.
> 
> > To workaround this issue, please use the x64 hosted toolset or use /cgthreads1 to disable multithreaded compilation.
> 
> It must be unacceptable for Windows devs to use /cgthreads1.
> Let's revert the patch until the upcoming MSVC release.

It's sad for progress to be gated by MSVC.  I'll roll out and see if I can workaround this for MSVC.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 202126

[Mark Lam]

Created attachment 381082 [details]
Rebased patch for landing.

[Mark Lam]

Created attachment 381108 [details]
refactored test patch for checking Windows build.

[Mark Lam]

Created attachment 381133 [details]
another test patch for MSVC.

[Mark Lam]

Created attachment 381134 [details]
test patch for MSVC.

[Mark Lam]

Created attachment 381135 [details]
test patch for MSVC.

[Mark Lam]

Created attachment 381142 [details]
patch for landing.

[Mark Lam]

Created attachment 381148 [details]
patch for landing.

[EWS Watchlist]

Comment on attachment 381148 [details]
patch for landing.

Attachment 381148 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13141860

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla

[Mark Lam]

(In reply to Build Bot from comment #22)
> New failing tests:
> mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla

I can reproduce this without my patch.  The test is inherently flaky.

[Mark Lam]

Fix landed in r251226: <http://trac.webkit.org/r251226>.

[Fujii Hironori]

Filed: Bug 203142 – [Clang][Windows] Options.cpp(317,25): error: no matching function for call to 'optionTypeSpecificIndex'

[Mark Lam]

This was rolled out in r251400: <http://trac.webkit.org/r251400>.

[Mark Lam]

Created attachment 382356 [details]
work in progress using type specific index from Options::s_optionsInfo.

[Mark Lam]

Created attachment 383711 [details]
proposed patch.

Without template magic this time.  In fact, I reduced the amount of stuff defined in Options.h.  So, compile time should be the same or marginally less.

[Mark Lam]

Created attachment 383712 [details]
proposed patch.

[Mark Lam]

Created attachment 383713 [details]
proposed patch.

[Robin Morisset]

Comment on attachment 383713 [details]
proposed patch.

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

r=me, I love a patch that makes things easier to read and more efficient at the same time.

> Source/JavaScriptCore/runtime/JSCConfig.h:42
> +constexpr size_t ConfigSizeToProtect = PageSize;

Is it possible to have some static_assert that the size of the options fit in that ?
I've seen one that checks for 16*KB, are we guaranteed that PageSize is always that on every platform?

> Source/JavaScriptCore/runtime/Options.cpp:917
>          return; // Illegal option.

If it is illegal, can we make it a release assert instead?

> Source/JavaScriptCore/runtime/Options.cpp:1038
>  {

nitpicking, but I think an ASSERT(type() == other.type()) might be valuable here.

> Source/JavaScriptCore/runtime/OptionsList.h:74
> +// Any modifications to options must be done before the first VM is instantiate.

nit: instantiate => instantiated.

> Source/JavaScriptCore/runtime/OptionsList.h:555
> +    { // Only needed for initialization

I don't understand this at all, this function appears to do nothing when passed a non-zero number. I realize it's not a change from your patch, but can you explain what this is for?

[Mark Lam]

Comment on attachment 383713 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/runtime/JSCConfig.h:42
>> +constexpr size_t ConfigSizeToProtect = PageSize;
> 
> Is it possible to have some static_assert that the size of the options fit in that ?
> I've seen one that checks for 16*KB, are we guaranteed that PageSize is always that on every platform?

We already have a static_assert(sizeof(Config) == ConfigSizeToProtect, "") below (in JSCConfig.h) that covers this.

>> Source/JavaScriptCore/runtime/Options.cpp:917
>>          return; // Illegal option.
> 
> If it is illegal, can we make it a release assert instead?

Done.

>> Source/JavaScriptCore/runtime/Options.cpp:1038
>>  {
> 
> nitpicking, but I think an ASSERT(type() == other.type()) might be valuable here.

Done.

>> Source/JavaScriptCore/runtime/OptionsList.h:74
>> +// Any modifications to options must be done before the first VM is instantiate.
> 
> nit: instantiate => instantiated.

Fixed.

>> Source/JavaScriptCore/runtime/OptionsList.h:555
>> +    { // Only needed for initialization
> 
> I don't understand this at all, this function appears to do nothing when passed a non-zero number. I realize it's not a change from your patch, but can you explain what this is for?

I think the reason for this is because options in OptionsList.h of type OptionRange has a default value of 0.  The 0 means no range specified, so please set to Uninitialized.  However, it's meaningless to pass in any other int value.  We can RELEASE_ASSERT that rhs is never non-zero, or provide a better way to specify the initial value.  I'll leave this as is for now.  We can improve it in a follow up patch.

[Mark Lam]

Landed in r252557: <http://trac.webkit.org/r252557>.