Bug 21727 - REGRESSION: the JavaScriptCore test js1_5/Regress/regress-159334.js takes extremely long to finish in Debug builds
Summary: REGRESSION: the JavaScriptCore test js1_5/Regress/regress-159334.js takes ext...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Gavin Barraclough
URL:
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2008-10-18 00:13 PDT by Cameron Zwarich (cpst)
Modified: 2008-10-23 15:31 PDT (History)
1 user (show)

See Also:


Attachments
The patch (29.18 KB, patch)
2008-10-23 12:48 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
changelog++ (31.42 KB, patch)
2008-10-23 14:28 PDT, Gavin Barraclough
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-10-18 00:13:33 PDT
A recent change has caused js1_5/Regress/regress-159334.js to take a very long time to run in debug builds, which makes the tests very inconvenient to run.
Comment 1 Geoffrey Garen 2008-10-20 10:25:22 PDT
The problem is that CodeBlock::getStubInfo is N^2, and Gavin's recent change to cache function calls caused much heavier reliance on it.

We should:

1. Change getStubInfo to use a hashtable, so it's no longer N^2
2. Stop using the structureIDInstructions vector to hold data other than instructions whose StructureIDs need reference counting, so structureIDInstructions returns to making sense.
Comment 2 Geoffrey Garen 2008-10-20 10:25:50 PDT
BTW, the same issue has cause a >10X regression on the empty function call benchmark's first run.

Comment 3 Gavin Barraclough 2008-10-20 13:24:43 PDT
(In reply to comment #1)
> The problem is that CodeBlock::getStubInfo is N^2, and Gavin's recent change to
> cache function calls caused much heavier reliance on it.
> 
> We should:
> 
> 1. Change getStubInfo to use a hashtable, so it's no longer N^2

Since the data is inherently sorted, simply binary chopping may make sense here, in the case of the call no search is actually necessary, we can simply pass a pointer to the function.

> 2. Stop using the structureIDInstructions vector to hold data other than
> instructions whose StructureIDs need reference counting, so
> structureIDInstructions returns to making sense.

I'm not sure I a benefit in holding the data required for property access caching in the interpreter and property access caching in CTI code in separate data structures, and it will mean performing two allocations to do so.  I do think a sensible division will be to move the information on calls into a separate data structure, since then these can be searched separately.  As such, we may want to look at renaming structureIDInstructions, since really I think we want this to be specifically data required for property access instructions.



Comment 4 Gavin Barraclough 2008-10-23 12:48:39 PDT
Created attachment 24609 [details]
The patch

Missing changelog, a wash on performance (other than in pathological case).
Comment 5 Gavin Barraclough 2008-10-23 14:28:34 PDT
Created attachment 24619 [details]
changelog++
Comment 6 Oliver Hunt 2008-10-23 14:40:16 PDT
Comment on attachment 24619 [details]
changelog++

r=me
Comment 7 Gavin Barraclough 2008-10-23 15:31:12 PDT
Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/VM/CTI.cpp
Sending        JavaScriptCore/VM/CTI.h
Sending        JavaScriptCore/VM/CodeBlock.cpp
Sending        JavaScriptCore/VM/CodeBlock.h
Sending        JavaScriptCore/VM/CodeGenerator.cpp
Sending        JavaScriptCore/VM/Machine.cpp
Transmitting file data .......
Committed revision 37831.