When looking at heap sizes for JS programs, UnlinkedFunctionExecutable often takes up large portions of the heap. Some ideas: - It shouldn't be a cell (8 bytes) - I don't think we really need the parentSourceOverride field since it's only used in one particular case (we can probably represent that with a bit). Then, we can probably regenerate source as needed (24 bytes) - We have 3 different name fields. I wonder if we really need 3. - We use 32-bits to store parameter count. Maybe we can make that 16bit. If we do this, we should probably do it in a way that parsing such a function is an OOM in the parser. - We store type profiling start offset, type profiling end offset, and the source length. Type profiling end offset is just typeProfilingStartOffset + length - 1. We should just compute it.
(In reply to Saam Barati from comment #0) > When looking at heap sizes for JS programs, UnlinkedFunctionExecutable often > takes up large portions of the heap. > > Some ideas: > - It shouldn't be a cell (8 bytes) Not gonna do this. It makes sense that it's a cell > - I don't think we really need the parentSourceOverride field since it's > only used in one particular case (we can probably represent that with a > bit). Then, we can probably regenerate source as needed (24 bytes) > - We have 3 different name fields. I wonder if we really need 3. > - We use 32-bits to store parameter count. Maybe we can make that 16bit. If > we do this, we should probably do it in a way that parsing such a function > is an OOM in the parser. > - We store type profiling start offset, type profiling end offset, and the > source length. Type profiling end offset is just typeProfilingStartOffset + > length - 1. We should just compute it.
I'm just going to remove the parentSourceOverride field. All we need is a bit to represent this. Not 24 bytes of a SourceCode.
Created attachment 340444 [details] patch
Comment on attachment 340444 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=340444&action=review r=me. Maybe it's worth adding a static_assert(sizeof(UnlinkedFunctionExecutable) <= 160, "Increasing the size of UnlinkedFunctionExecutable means that it increases its size class");? > Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:66 > + case ConstructorKind::Extends: > + static NeverDestroyed<const String> derivedConstructorCode(MAKE_STATIC_STRING_IMPL("(function (...args) { super(...args); })")); > + static LazyNeverDestroyed<SourceCode> result; > + static std::once_flag onceFlag; > + std::call_once(onceFlag, [&] { > + result.construct(makeSource(derivedConstructorCode, { })); > + }); > + return result; Nit: put { } around the case since you are allocating variables. > Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:77 > - return createExecutable(m_vm, makeSource(baseConstructorCode, { }), name, constructorKind, ConstructAbility::CanConstruct); > + return createExecutable(m_vm, defaultConstructorSourceCode(constructorKind), name, constructorKind, ConstructAbility::CanConstruct); I think you can get rid of this line.
Comment on attachment 340444 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=340444&action=review >> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:66 >> + return result; > > Nit: put { } around the case since you are allocating variables. sounds good. >> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:77 >> + return createExecutable(m_vm, defaultConstructorSourceCode(constructorKind), name, constructorKind, ConstructAbility::CanConstruct); > > I think you can get rid of this line. good call
Created attachment 340545 [details] patch for landing
Comment on attachment 340545 [details] patch for landing Clearing flags on attachment: 340545 Committed r231889: <https://trac.webkit.org/changeset/231889>
All reviewed patches have been landed. Closing bug.
<rdar://problem/40321629>
This caused a build failure on Windows (from an EWS build on another patch): c:\cygwin\home\buildbot\webkit\source\javascriptcore\builtins\builtinexecutables.cpp(69): error C2220: warning treated as error - no 'object' file generated [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj] c:\cygwin\home\buildbot\webkit\source\javascriptcore\builtins\builtinexecutables.cpp(69): warning C4715: 'JSC::BuiltinExecutables::defaultConstructorSourceCode': not all control paths return a value [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
(In reply to David Kilzer (:ddkilzer) from comment #10) > This caused a build failure on Windows (from an EWS build on another patch): > > c: > \cygwin\home\buildbot\webkit\source\javascriptcore\builtins\builtinexecutable > s.cpp(69): error C2220: warning treated as error - no 'object' file > generated > [C: > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\JavaScriptCore\JavaSc > riptCore.vcxproj] > c: > \cygwin\home\buildbot\webkit\source\javascriptcore\builtins\builtinexecutable > s.cpp(69): warning C4715: > 'JSC::BuiltinExecutables::defaultConstructorSourceCode': not all control > paths return a value > [C: > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\JavaScriptCore\JavaSc > riptCore.vcxproj] Will attempt a commit to fix in ~5 minutes
windows build fix: https://trac.webkit.org/changeset/231901/webkit
(In reply to Saam Barati from comment #12) > windows build fix: > https://trac.webkit.org/changeset/231901/webkit Fix the build after this windows attempt broke the build: https://trac.webkit.org/changeset/231904/webkit