Currently, the PIC generates a fast path for string length only for things tagged as strings. But we can also get objects that wrap a string (e.g., from new String()). The PIC should generate a fast path for that, too. I think it would be sufficient to generate a stub that handles one case based on the current value, but it might be better to generate a stub that handles both.

On callgrind I'm seeing 20K calls to mjit::stubs::Length in date-format-xparb, costing 5.3M insns. We execute 15M more insns than jsc on this benchmark, so that (2ms on my machine), so this might be worth half a ms. (Note that its not just that we take the slow path, but that we go through InlineGetProp (4.5M insns).)

This PIC helps on my microbenchmark (about 120x times faster), but i can't see improvements in date-format-xparb.

(In reply to comment #2) Nice work! > This PIC helps on my microbenchmark (about 120x times faster), but i can't see > improvements in date-format-xparb. I can observe a 7M (12%) insn count drop and 4.2M (14%) data ref count drop in cachegrind. Also, while the variance of total SS obscures the change, I get a reliable speedup on the individual benchmark: format-xparb: 1.22x as fast 16.0ms +/- 2.1% 13.1ms +/- 1.7%

Comment on attachment 494397 [details] [diff] [review] PIC for string object length Nice, thanks for doing this! This patch wants at least two tests cases, one for normal string objects and one for where obj->slots has been dynamically allocated. >+ masm.loadPtr(Address(pic.objReg, offsetof(JSObject, slots)), pic.objReg); There's a load missing load, you want something like this right after: masm.loadPayload(Address(pic.objReg, JSObject::JSSLOT_PRIMITIVE_THIS * sizeof(Value)), pic.objReg) Without it you'll fail this test case: > function f(s) { > return new String(s).length; > } > > assertEq(f("x"), 1); // Warm-up > assertEq(f("x"), 1); // Create IC > assertEq(f("x"), 1); // Test IC A similar test case that adds a bunch more slots to the object: > function g(s) { > var x = new String(s); > for (var i = 0; i < 100; i++) > x[i] = i; > return x.length; > } > ... > It's good to have this one too, because (although it won't matter in practice) you could imagine eliminating the extra load someday. (new String("x")) will start off with "fixed" slots, meaning the slot is right after the JSObject. But if some really weird code adds a bunch of properties, it'll get dynamically allocated slots. Your code handles both - but if you wanted to guard on the shape, you could lose the indirection in the most common case.

In my first version, i had this extra load, but i concluded if i wanted to load the first slot i won't need it. So can use this, as JSObject::JSSLOT_PRIMITIVE_THIS * sizeof(Value) would be 0? >masm.loadPtr(pic.objReg, pic.objReg); If not tell me something about this loadPayload.

Unfortunately the extra step is needed, first to read obj->slots, and second to read the JSString * inside that, since obj->slots is a Value *. The loadPayload is a helper function. A Value is a pair of (Type, Payload). On some platforms (like X64) the encoding of a Value requires some bit trickery. loadPayload() will do that for you, whereas loadPtr won't. Yeah, JSSLOT_PRIMITIVE_THIS * sizeof(Value) will be 0, effectively obj->slots[0].toString(). But please use the full symbol in case someone changes it ;)

Although a new String instance could grow enough slots to need out-of-line non-fixedSlots, if that happens, won't the immutable value of JSSLOT_PRIMITIVE_THIS still be left in stringObj->fixedSlots()[0]? IOW, no need to load obj->slots? Cc'ing bhackett. /be

Hrmm, if String instances are always initially allocated with inline slots, going straight to fixedSlots should be OK.

Comment on attachment 495072 [details] [diff] [review] PIC for string object length v2 Nice work! r=me, but this should have tests to land (at the least, ones from comment #4).

Sorry for spam