Bug 108033 – JSC: FunctionParameters are memory hungry.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Andreas Kling]

JSC's FunctionParameters currently inherit from Vector<Identifier>. Since the objects never mutate after creation, this is quite wasteful.

[Andreas Kling]

Created attachment 184916 [details]
Proposed patch

[WebKit Review Bot]

Attachment 184916 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp', u'Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp', u'Source/JavaScriptCore/parser/Nodes.cpp', u'Source/JavaScriptCore/parser/Nodes.h']" exit_code: 1
Source/JavaScriptCore/parser/Nodes.cpp:171:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 184916 [details]
Proposed patch

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

> Source/JavaScriptCore/parser/Nodes.cpp:161
> +    void* slot = WTF::fastMalloc(objectSize);

Again with the WTF::  (Do you need it?)

>> Source/JavaScriptCore/parser/Nodes.cpp:171
>> +    for (ParameterNode* parameter = firstParameter; parameter; parameter = parameter->nextParam()) {
>> +        new (&identifiers()[i++]) Identifier(parameter->ident());
>> +    }
> 
> One line control clauses should not use braces.  [whitespace/braces] [4]

No need for braces here.

[Radar WebKit Bug Importer]

<rdar://problem/13094803>

[Andreas Kling]

Committed r140947: <http://trac.webkit.org/changeset/140947>

[Darin Adler]

Comment on attachment 184916 [details]
Proposed patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:363
> -            if (!functionBody->captures(parameters[i]) && !shouldCaptureAllTheThings)
> +            if (!functionBody->captures(parameters.at(i)) && !shouldCaptureAllTheThings)

We could have overridden operator[] and then we would not have had to change these at the call sites.

> Source/JavaScriptCore/parser/Nodes.cpp:160
> +    size_t objectSize = sizeof(FunctionParameters) - sizeof(void*) + sizeof(StringImpl*) * parameterCount;

Why does this calculation use StringImpl* as the type, but the rest of the code uses Identifier?

> Source/JavaScriptCore/parser/Nodes.cpp:162
> +    return adoptRef(new (slot) FunctionParameters(firstParameter, parameterCount));

Is it really OK to return something allocated with fastMalloc, if the caller is going to delete the object with delete? I believe that technically if we allocate this way, then we need to delete with a combination of an explicit destructor call and a call to fastFree.

> Source/JavaScriptCore/parser/Nodes.h:1412
> +        void* m_storage;

I am surprised that here we used void* as the type for m_storage, but in the other recent patch we used a zero-sized array for the same idiom.

[Oliver Hunt]

(In reply to comment #6)
> (From update of attachment 184916 [details])

> > Source/JavaScriptCore/parser/Nodes.cpp:162
> > +    return adoptRef(new (slot) FunctionParameters(firstParameter, parameterCount));
> 
> Is it really OK to return something allocated with fastMalloc, if the caller is going to delete the object with delete? I believe that technically if we allocate this way, then we need to delete with a combination of an explicit destructor call and a call to fastFree.
>

Yeah, if you turn on malloc validation it will complain about mismatching malloc/new<->delete/free, one day i'll have validation on by default in debug builds, but currently there are too many places in webcore where everything is filled with sadness.

--Oliver

[Andreas Kling]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 184916 [details] [details])
> 
> > > Source/JavaScriptCore/parser/Nodes.cpp:162
> > > +    return adoptRef(new (slot) FunctionParameters(firstParameter, parameterCount));
> > 
> > Is it really OK to return something allocated with fastMalloc, if the caller is going to delete the object with delete? I believe that technically if we allocate this way, then we need to delete with a combination of an explicit destructor call and a call to fastFree.
> >
> 
> Yeah, if you turn on malloc validation it will complain about mismatching malloc/new<->delete/free, one day i'll have validation on by default in debug builds, but currently there are too many places in webcore where everything is filled with sadness.

I thought that the "operator delete" added by WTF_MAKE_FAST_ALLOCATED ensured that fastFree() gets called as appropriate. Is that not the case?

[Andreas Kling]

(In reply to comment #6)
> (From update of attachment 184916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184916&action=review
> 
> > Source/JavaScriptCore/parser/Nodes.cpp:160
> > +    size_t objectSize = sizeof(FunctionParameters) - sizeof(void*) + sizeof(StringImpl*) * parameterCount;
> 
> Why does this calculation use StringImpl* as the type, but the rest of the code uses Identifier?

That is indeed a mistake.

> > Source/JavaScriptCore/parser/Nodes.h:1412
> > +        void* m_storage;
> 
> I am surprised that here we used void* as the type for m_storage, but in the other recent patch we used a zero-sized array for the same idiom.

That would be cleaner. I'll make a patch to tidy this up.

[Oliver Hunt]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 184916 [details] [details] [details])
> > 
> > > > Source/JavaScriptCore/parser/Nodes.cpp:162
> > > > +    return adoptRef(new (slot) FunctionParameters(firstParameter, parameterCount));
> > > 
> > > Is it really OK to return something allocated with fastMalloc, if the caller is going to delete the object with delete? I believe that technically if we allocate this way, then we need to delete with a combination of an explicit destructor call and a call to fastFree.
> > >
> > 
> > Yeah, if you turn on malloc validation it will complain about mismatching malloc/new<->delete/free, one day i'll have validation on by default in debug builds, but currently there are too many places in webcore where everything is filled with sadness.
> 
> I thought that the "operator delete" added by WTF_MAKE_FAST_ALLOCATED ensured that fastFree() gets called as appropriate. Is that not the case?

fastFree will be called, however it is generally undefined behaviour to mismatch the allocation and release mechanisms.  But alas we get somewhat hosed due to our use of in place and regular object allocation :-/