Closed Bug 557353 Opened 11 years ago Closed 11 years ago

JM: PIC for object-wrapped string length

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b9

People

(Reporter: dmandelin, Assigned: evilpie)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 4 obsolete files)

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).)
Attached patch PIC for string object length (obsolete) — Splinter Review
This PIC helps on my microbenchmark (about 120x times faster), but i can't see improvements in date-format-xparb.
Attachment #494397 - Flags: review?(dvander)
(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%
Attachment #494397 - Attachment is patch: true
Attachment #494397 - Attachment mime type: application/octet-stream → text/plain
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.
Attachment #494397 - Flags: review?(dvander)
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.
Attached patch PIC for string object length v2 (obsolete) — Splinter Review
Attachment #495072 - Flags: review?(dvander)
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).
Attachment #495072 - Flags: review?(dvander) → review+
Attached file PIC for string object length (obsolete) —
Assignee: general → evilpies
Attachment #494397 - Attachment is obsolete: true
Attachment #495072 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #500287 - Flags: review+
Attachment #500287 - Attachment is obsolete: true
Sorry for spam
Attachment #500288 - Attachment is obsolete: true
http://hg.mozilla.org/tracemonkey/rev/feb28ec64b74
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.0b9
http://hg.mozilla.org/mozilla-central/rev/feb28ec64b74
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 623474
You need to log in before you can comment on or make changes to this bug.