Bug 185637

Summary: UnlinkedFunctionExecutable doesn't need a parent source override field since it's only used for default class constructors
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, ddkilzer, fpizlo, ggaren, gskachkov, jfbastien, jlewis3, keith_miller, mark.lam, msaboff, rmorisset, ticaiolima, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
keith_miller: review+
patch for landing none

Saam Barati
Reported 2018-05-14 18:17:59 PDT
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.
Attachments
patch (15.43 KB, patch)
2018-05-15 16:35 PDT, Saam Barati 		keith_miller: review+
DetailsFormatted DiffDiff
patch for landing (15.82 KB, patch)
2018-05-16 17:50 PDT, Saam Barati 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Saam Barati
Comment 1 2018-05-14 19:01:32 PDT
(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.
Saam Barati
Comment 2 2018-05-15 16:16:55 PDT
I'm just going to remove the parentSourceOverride field. All we need is a bit to represent this. Not 24 bytes of a SourceCode.
Saam Barati
Comment 3 2018-05-15 16:35:02 PDT
Created attachment 340444 [details] patch
Keith Miller
Comment 4 2018-05-15 21:49:09 PDT
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.
Saam Barati
Comment 5 2018-05-16 17:36:53 PDT
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
Saam Barati
Comment 6 2018-05-16 17:50:54 PDT
Created attachment 340545 [details] patch for landing
WebKit Commit Bot
Comment 7 2018-05-16 22:21:47 PDT
Comment on attachment 340545 [details] patch for landing Clearing flags on attachment: 340545 Committed r231889: <https://trac.webkit.org/changeset/231889>
WebKit Commit Bot
Comment 8 2018-05-16 22:21:48 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 9 2018-05-16 22:22:21 PDT
<rdar://problem/40321629>
David Kilzer (:ddkilzer)
Comment 10 2018-05-17 04:46:05 PDT
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]
Saam Barati
Comment 11 2018-05-17 08:57:40 PDT
(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
Saam Barati
Comment 12 2018-05-17 09:04:03 PDT
windows build fix: https://trac.webkit.org/changeset/231901/webkit
Saam Barati
Comment 13 2018-05-17 09:17:30 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.