You need to
before you can comment on or make changes to this bug.
The main JSC JIT has caching optimizations for get_by_id for array length. The DFG JIT lacks these optimizations, and instead has to use a slow path C call for all array length accesses.
Created an attachment (id=98848) [details]
Attachment 98848 [details] did not pass style-queue:
Total errors found: 8 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created an attachment (id=98849) [details]
the patch (fix style)
(From update of attachment 98849 [details])
Attachment 98849 [details] did not pass qt-ews (qt):
Created an attachment (id=98860) [details]
the patch (fix build)
(From update of attachment 98860 [details])
Attachment 98860 [details] did not pass gtk-ews (gtk):
Created an attachment (id=98973) [details]
the patch (fix build)
(From update of attachment 98973 [details])
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 an attachment (id=99025) [details]
Assembler modifications removed - they did not prove to produce any speed-up over the simpler approach.
(From update of attachment 99025 [details])
r+, I'll land.
Fixed in r89986