Bug 28701 - How many copies of the parameters do you need?
Summary: How many copies of the parameters do you need?
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
Depends on:
Reported: 2009-08-24 21:56 PDT by Gavin Barraclough
Modified: 2009-09-01 03:04 PDT (History)
1 user (show)

See Also:

The Patch (13.00 KB, patch)
2009-08-24 22:03 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
PassRefPtr != RefPtr (12.99 KB, patch)
2009-08-24 22:09 PDT, Gavin Barraclough
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2009-08-24 21:56:23 PDT
The function parameters in JSC get copied a lot - and unnecessarily so.

Originally this happened due to duplicating FunctionBodyNodes on recompilation, though the problem has been exacerbated by copying the parameters from the original function body onto the executable, then back onto the real body that will be generated (this happens on every function).  And this is all made worse since the data structures in question are a little ugly - C style arrays of C++ objects containing ref counts, so they need a full copy-construct (rather than a simple memcpy).

This can all be greatly simplified by just punting the parameters off into their own ref-counted object, and forgoing all the copying.
Comment 1 Gavin Barraclough 2009-08-24 22:03:09 PDT
Created attachment 38523 [details]
The Patch
Comment 2 Gavin Barraclough 2009-08-24 22:09:59 PDT
Created attachment 38524 [details]
PassRefPtr != RefPtr
Comment 3 Darin Adler 2009-08-25 15:10:51 PDT
Comment on attachment 38524 [details]
PassRefPtr != RefPtr

> +    finishParsing(adoptRef(new FunctionParameters(firstParameter)), ident);

The usual idiom would be to make a create function instead of doing the adoptRef here. I'd prefer to see it done that way.

I'd like to see the same thing for FunctionExecutable too; lets keep those adoptRef inside create functions, and keep the constructors private.

> +        void finishParsing(PassRefPtr<FunctionParameters> parameters, const Identifier& ident);

You could omit both of these argument names.

>  FunctionExecutable::~FunctionExecutable()
>  {
> -    for (int i = 0; i < m_parameterCount; ++i)
> -        m_parameters[i].~Identifier();
> -    fastFree(m_parameters);
>      delete m_codeBlock;
>  }

Seems like we should use OwnPtr for m_codeBlock.

> +    return adoptRef(new FunctionExecutable(functionName, body->source(), body->usesArguments(), body->parameters(), body->lineNo(), body->lastLine()));

See above comment about create.

Comment 4 Gavin Barraclough 2009-08-26 00:51:28 PDT
Fixed all review comments except the OwnPtr one.

The problem here is that CodeBlock includes Executable, not vice-versa.  Every CodeBlock has a pointer to its owner Executable, and contains inline methods that access the it.  The OwnPtr template is not satisfied by a forward declaration of the CodeBlock types, so manual deletion seemed the simplest solution.

(Though any suggestions as to how this can be resolved would be welcomed.)

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
Sending        JavaScriptCore/bytecompiler/BytecodeGenerator.h
Sending        JavaScriptCore/parser/Nodes.cpp
Sending        JavaScriptCore/parser/Nodes.h
Sending        JavaScriptCore/runtime/Executable.cpp
Sending        JavaScriptCore/runtime/Executable.h
Transmitting file data .......
Committed revision 47775.
Comment 5 Eric Seidel (no email) 2009-08-26 03:10:31 PDT
Looks like this caused:

to fail on the bots.  I'll re-open this for now in case we decide to roll this out.
Comment 6 Eric Seidel (no email) 2009-09-01 03:04:28 PDT
I guess the test got fixed later, since I see no indication of this being rolled out.  closing again.