Bug 22480 - We should cache get by id chain accesses polymorphically.
Summary: We should cache get by id chain accesses polymorphically.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-24 23:18 PST by Gavin Barraclough
Modified: 2008-11-25 15:07 PST (History)
0 users

See Also:


Attachments
The patch (31.11 KB, patch)
2008-11-24 23:19 PST, Gavin Barraclough
ggaren: 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 2008-11-24 23:18:52 PST
deltablue spends 7-10% of it's time doing these.
Comment 1 Gavin Barraclough 2008-11-24 23:19:31 PST
Created attachment 25475 [details]
The patch

        Polymorpic caching for get by id chain.  Similar to the polymorphic caching already implemented
        for self and proto accesses (implemented by allowing multiple trampolines to be JIT genertaed,
        and linked together) - the get by id chain caching is implemented as a genericization of the
        proto list caching, allowing cached access lists to contain a mix of proto and proto chain
        accesses (since in JS style inheritance hierarchies you may commonly see a mix of properties
        being overridden on the direct prototype, or higher up its prototype chain).

        In order to allow this patch to compile there is a fix to appease gcc 4.2 compiler issues
        (removing the jumps between fall-through cases in privateExecute).
        
        This patch also removes redundant immediate checking from the reptach code, and fixes a related
        memory leak (failure to deallocate trampolines). 

        ~2% progression on v8 tests (bulk on the win on deltablue)
Comment 2 Geoffrey Garen 2008-11-25 13:23:34 PST
Comment on attachment 25475 [details]
The patch

> +    } else if (vPC[0].u.opcode == interpreter->getOpcode(op_get_by_id_chain)) {
> +        prototypeStructureList = new PolymorphicAccessStructureList(vPC[6].u.operand, stubInfo->stubRoutine, vPC[4].u.structure, vPC[5].u.structureChain);
> +        stubInfo->stubRoutine = 0;
> +
> +        vPC[0] = interpreter->getOpcode(op_get_by_id_proto_list);
> +        vPC[4] = prototypeStructureList;
> +        vPC[5] = 2;
> +    } else {

Should ASSERT here which opcode is in vPC[0].

> -            vPC[0] = ARG_globalData->interpreter->getOpcode(op_get_by_id_proto_list);
> -            vPC[4] = prototypeStructureList;
> -            vPC[5] = 2;
> -        } else {
> -            prototypeStructureList = vPC[4].u.polymorphicStructures;
> -            listIndex = vPC[5].u.operand;
> +            StructureChain* chain = structure->cachedPrototypeChain();
> +            if (!chain)
> +                chain = cachePrototypeChain(callFrame, structure);
> +            ASSERT(chain);
>  
> -            vPC[5] = listIndex + 1;
> -        }
> +        StructureStubInfo* stubInfo = &codeBlock->getStubInfo(CTI_RETURN_ADDRESS);
> +        int listIndex;
> +        PolymorphicAccessStructureList* prototypeStructureList = getPolymorphicAccessStructureListSlot(ARG_globalData->interpreter, stubInfo, vPC, listIndex);

Incorrect indentation here? Is "chain" ever used? Why do we compute it?

I hope we can refactor some of this code soon to make the memory management automatic.

It would be nice to eventually add a feature for re-caching by starting over from scratch in the case where the prototype has changed.

r=me
Comment 3 Gavin Barraclough 2008-11-25 15:07:51 PST
Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/bytecode/Instruction.h
Sending        JavaScriptCore/interpreter/Interpreter.cpp
Sending        JavaScriptCore/interpreter/Interpreter.h
Sending        JavaScriptCore/jit/JIT.cpp
Sending        JavaScriptCore/jit/JIT.h
Transmitting file data ......
Committed revision 38763.