Summary: | DFG JIT lacks array.length caching | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | barraclough, gustavo.noronha, gustavo, webkit.review.bot, xan.lopez | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Filip Pizlo
2011-06-27 20:02:13 PDT
Created attachment 98848 [details]
the patch
Attachment 98848 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/dfg/DFGRegisterBank.h:83: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:32: Alphabetical sorting problem. [build/include_order] [4]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:33: Alphabetical sorting problem. [build/include_order] [4]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:104: One line control clauses should not use braces. [whitespace/braces] [4]
Source/JavaScriptCore/dfg/DFGRepatch.cpp:114: One line control clauses should not use braces. [whitespace/braces] [4]
Source/JavaScriptCore/assembler/X86Assembler.h:1577: Missing spaces around << [whitespace/operators] [3]
Source/JavaScriptCore/assembler/X86Assembler.h:1584: Missing spaces around >> [whitespace/operators] [3]
Source/JavaScriptCore/assembler/X86Assembler.h:1588: Missing spaces around << [whitespace/operators] [3]
Total errors found: 8 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 98849 [details]
the patch (fix style)
Comment on attachment 98849 [details] the patch (fix style) Attachment 98849 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8952420 Created attachment 98860 [details]
the patch (fix build)
Comment on attachment 98860 [details] the patch (fix build) Attachment 98860 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8954277 Created attachment 98973 [details]
the patch (fix build)
Comment on attachment 98973 [details]
the patch (fix build)
Hey Filip, 2 issues.
First, I'd like like to keep the JIT implementation separate from the C helper functions, so the code that performs JIT generation shouldn't be in DFGOperations - that's really what the file DFGRepatch is for. (The stub generation could even be in a new DFGStubs file if we wished, but DFGRepatch seems right for patching the instruction stream). Please move the new code in tryCacheGetByID out into a separate function.
Second, this patch introduces a bunch of new code to allow us to overwrite the instruction stream, which is a little messy. I'm okay with us doing this provided that there is a benefit to doing so, but it seems like a bad idea to introduce this complexity if it's not a performance win, and I think you need to prove this. I think you need to gather performance numbers with this patch, and I think you need to also roll a version of this optimization that just patches the offset of the branch instruction (as the old JIT does), and attach a performance comparison (you can get this by running sun spider in both forms, cat'ing the results to files, then using the sunspider-compare-results script passing the two results files as arguments).
Hope this all sounds reasonable.
Array length caching is mainly for v8, not sunspider, and is a win (see attached). The replaceWithJump portion is not a win on either v8 or sunspider, so I've removed it. TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: 1.038x as fast 1248.1ms +/- 0.5% 1202.2ms +/- 0.3% significant ============================================================================= v8: 1.038x as fast 1248.1ms +/- 0.5% 1202.2ms +/- 0.3% significant crypto: - 92.8ms +/- 0.8% 92.3ms +/- 0.4% deltablue: 1.148x as fast 329.7ms +/- 0.4% 287.1ms +/- 0.5% significant earley-boyer: - 149.7ms +/- 1.3% 147.6ms +/- 1.0% raytrace: - 102.8ms +/- 3.6% 101.3ms +/- 1.0% regexp: - 117.5ms +/- 0.6% 117.2ms +/- 0.4% richards: ?? 245.3ms +/- 0.9% 245.7ms +/- 0.7% not conclusive: might be *1.002x as slow* splay: ?? 210.3ms +/- 0.7% 211.0ms +/- 0.8% not conclusive: might be *1.003x as slow* Created attachment 99025 [details]
the patch
Assembler modifications removed - they did not prove to produce any speed-up over the simpler approach.
Comment on attachment 99025 [details]
the patch
r+, I'll land.
Fixed in r89986 |