WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
108033
JSC: FunctionParameters are memory hungry.
https://bugs.webkit.org/show_bug.cgi?id=108033
Summary
JSC: FunctionParameters are memory hungry.
Andreas Kling
Reported
2013-01-27 09:32:18 PST
JSC's FunctionParameters currently inherit from Vector<Identifier>. Since the objects never mutate after creation, this is quite wasteful.
Attachments
Proposed patch
(6.71 KB, patch)
2013-01-27 09:33 PST
,
Andreas Kling
sam
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Andreas Kling
Comment 1
2013-01-27 09:33:14 PST
Created
attachment 184916
[details]
Proposed patch
WebKit Review Bot
Comment 2
2013-01-27 09:35:56 PST
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 3
2013-01-27 21:53:41 PST
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
Comment 4
2013-01-27 22:00:35 PST
<
rdar://problem/13094803
>
Andreas Kling
Comment 5
2013-01-27 22:48:15 PST
Committed
r140947
: <
http://trac.webkit.org/changeset/140947
>
Darin Adler
Comment 6
2013-01-28 09:45:14 PST
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
Comment 7
2013-01-28 09:55:18 PST
(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
Comment 8
2013-01-28 10:04:27 PST
(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
Comment 9
2013-01-28 10:10:05 PST
(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
Comment 10
2013-01-28 10:12:09 PST
(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 :-/
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug