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+
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
Andreas Kling
Comment 5 2013-01-27 22:48:15 PST
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.