Bug 47107 - Lazily create activation objects
Summary: Lazily create activation objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
: 47718 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-10-04 13:23 PDT by Oliver Hunt
Modified: 2011-06-14 19:05 PDT (History)
1 user (show)

See Also:


Attachments
Patch (42.51 KB, patch)
2010-10-04 13:30 PDT, Oliver Hunt
ggaren: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2010-10-04 13:23:21 PDT
Lazily create activation objects
Comment 1 Oliver Hunt 2010-10-04 13:30:19 PDT
Created attachment 69674 [details]
Patch
Comment 2 Geoffrey Garen 2010-10-04 13:53:33 PDT
Comment on attachment 69674 [details]
Patch

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

> JavaScriptCore/interpreter/Interpreter.cpp:3173
> -            callFrame->r(dst) = JSValue(arguments);
> -            callFrame->r(unmodifiedArgumentsRegister(dst)) = JSValue(arguments);
> +            callFrame->r(argumentsRegister) = JSValue(arguments);
> +            callFrame->r(unmodifiedArgumentsRegister(argumentsRegister)) = JSValue(arguments);

Since this turned out to be just a bug fix for a bug in a previous arguments optimization patch, I think you should land it separately with a test case.
Comment 3 Darin Adler 2010-10-04 13:57:47 PDT
Comment on attachment 69674 [details]
Patch

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

> JavaScriptCore/ChangeLog:13
> +        This does make exception handling a little more dicey as

The word “dicey” here makes me nervous. I wish you had said it differently.

Is there enough test coverage?

> JavaScriptCore/bytecode/CodeBlock.h:433
> +        void createActivation(CallFrame*);
>  #if ENABLE(INTERPRETER)

Why no blank line here? I would have had one.

> JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1572
> +void BytecodeGenerator::createActivationIfNecessary()
> +{
> +    if (m_hasCreatedActivation)
> +        return;
> +    if (!m_codeBlock->needsFullScopeChain())
> +        return;
> +    emitOpcode(op_create_activation);
> +    instructions().append(m_activationRegister->index());
> +}

Should this function set m_hasCreatedActivation to true? If not, why not? It might need a comment.

> JavaScriptCore/interpreter/Interpreter.cpp:131
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/interpreter/Interpreter.cpp:213
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/interpreter/Interpreter.cpp:2356
> +        if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITOpcodes.cpp:472
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITOpcodes.cpp:494
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITOpcodes.cpp:1590
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITOpcodes32_64.cpp:614
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITOpcodes32_64.cpp:643
> +    if (checkTopLevel && skip--) {

Same problem here that I spotted below.

> JavaScriptCore/jit/JITStubs.cpp:2697
> +    if (checkTopLevel && skip--) {

If skip is 0 this will set it to -1. I don’t think you want that.
Comment 4 Oliver Hunt 2010-10-04 15:43:37 PDT
Committed r69045
Comment 5 Gavin Barraclough 2011-06-14 19:05:47 PDT
*** Bug 47718 has been marked as a duplicate of this bug. ***