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 <sbarati>
Component: JavaScriptCoreAssignee: Saam Barati <sbarati>
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

Description Saam Barati 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.
Comment 1 Saam Barati 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.
Comment 2 Saam Barati 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.
Comment 3 Saam Barati 2018-05-15 16:35:02 PDT
Created attachment 340444 [details]
patch
Comment 4 Keith Miller 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.
Comment 5 Saam Barati 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
Comment 6 Saam Barati 2018-05-16 17:50:54 PDT
Created attachment 340545 [details]
patch for landing
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2018-05-16 22:21:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-05-16 22:22:21 PDT
<rdar://problem/40321629>
Comment 10 David Kilzer (:ddkilzer) 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]
Comment 11 Saam Barati 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
Comment 12 Saam Barati 2018-05-17 09:04:03 PDT
windows build fix:
https://trac.webkit.org/changeset/231901/webkit
Comment 13 Saam Barati 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