This patch inlines .charAt and .charCodeAt which a lot of SS tests use. Individually this isn't much of the runtime of each test, but the total is pretty significant. Together with a second patch I am about to post we speed up SS by 6.7% (six point seven). The two patches work together particularly efficiently because with both patches we don't call any builtins in a bunch of paths with greatly improves code quality and reduces spilling.

Comment on attachment 456711 [details] [diff] [review] patch Actually I think nick should review this. The 16-bit LIR load stuff is weird and he knows that code well.

Comment on attachment 456711 [details] [diff] [review] patch >-static JSBool >-str_charAt(JSContext *cx, uintN argc, jsval *vp) >+JSBool >+js_str_charAt(JSContext *cx, uintN argc, jsval *vp) Can this be moved into the js:: namespace instead? >+JS_REQUIRES_STACK LIns* >+TraceRecorder::getCharCodeAt(LIns* str_ins, LIns* idx_ins, VMSideExit* exit) >+{ >+ idx_ins = lir->insUI2P(makeNumberInt32(idx_ins)); >+ LIns *length_ins = lir->insLoad(LIR_ldp, str_ins, offsetof(JSString, mLength), ACC_READONLY); >+ LIns *chars_ins = lir->insLoad(LIR_ldp, str_ins, offsetof(JSString, mChars), ACC_READONLY); >+ >+ guard(true, lir->ins2(LIR_ltup, idx_ins, length_ins), exit); >+ >+ return lir->insLoad(LIR_lduc2ui, >+ lir->ins2(LIR_addp, chars_ins, lir->ins2ImmI(LIR_lshp, idx_ins, 1)), >+ 0, ACC_READONLY); >+} Why LIR_lduc2ui? mChars is an array of jschar, which is uint16, so surely this should be LIR_ldus2ui? This looks too simple. It's replacing str_charAt(), right? AFAICT if you see a string at record-time you assume you'll always get a string as input. Shouldn't there be a type guard (or two)? >+ >+JS_REQUIRES_STACK LIns* >+TraceRecorder::getCharAt(LIns* str_ins, LIns* idx_ins, VMSideExit* exit) >+{ >+ LIns* ch_ins = getCharCodeAt(str_ins, idx_ins, exit); >+ >+ guard(true, lir->ins2ImmI(LIR_ltui, ch_ins, UNIT_STRING_LIMIT), exit); >+ >+ LIns *unitstr_ins = lir->ins2(LIR_addp, >+ INS_CONSTPTR(JSString::unitStringTable), >+ lir->ins2ImmI(LIR_lshp, >+ lir->insUI2P(ch_ins), >+ (sizeof(JSString) == 16) ? 4 : 5)); I don't understand that ternary expression. You don't have a single comment in the patch, one would be nice here. You should also have some kind of assertion that 4 and 5 are the only appropriate values.

(In reply to comment #3) > > > >+JS_REQUIRES_STACK LIns* > >+TraceRecorder::getCharCodeAt(LIns* str_ins, LIns* idx_ins, VMSideExit* exit) > >+{ > >+ idx_ins = lir->insUI2P(makeNumberInt32(idx_ins)); > >+ LIns *length_ins = lir->insLoad(LIR_ldp, str_ins, offsetof(JSString, mLength), ACC_READONLY); > >+ LIns *chars_ins = lir->insLoad(LIR_ldp, str_ins, offsetof(JSString, mChars), ACC_READONLY); > >+ > >+ guard(true, lir->ins2(LIR_ltup, idx_ins, length_ins), exit); > >+ > >+ return lir->insLoad(LIR_lduc2ui, > >+ lir->ins2(LIR_addp, chars_ins, lir->ins2ImmI(LIR_lshp, idx_ins, 1)), > >+ 0, ACC_READONLY); > >+} Also, are you assuming that the string is flat? If it's dependent I think you can't directly access mChars like that. (I don't know much about JSString, but just looking at chars() that's the impression I got.)

David, is it worth doing something similar in JM? See also discussion in bug 576926. Bug needed?

Definitely - v8 manages to do it (I suspect it's self-hosted, and just goes through a faster call mechanism), so we can figure something out too. I'll file a follow-up bug on PICs for calls later today.

Comment on attachment 456711 [details] [diff] [review] patch Marking r- to clean up my review queue. I'm happy to review a revised patch.

If this was a 6.7% win on TM, would it not be useful for JM+TM?

See bug 579574 (and also see bug 582789 comment #4).

So, this should be marked WONTFIX, yes?

(In reply to comment #10) > So, this should be marked WONTFIX, yes? Looks like the changes here were subsumed by those in bug 584499. So I'll mark it as WONTFIX.