UnlinkedFunctionExecutable doesn't need a parent source override field since it's only used for default class constructors

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

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.

[Saam Barati]

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

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]

Created attachment 340444 [details]
patch

[Keith Miller]

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

Created attachment 340545 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 340545 [details]
patch for landing

Clearing flags on attachment: 340545

Committed r231889: <https://trac.webkit.org/changeset/231889>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40321629>

[David Kilzer (:ddkilzer)]

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]

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

windows build fix:
https://trac.webkit.org/changeset/231901/webkit

[Saam Barati]

(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