JM: PIC for object-wrapped string length

RESOLVED FIXED in mozilla2.0b9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: dmandelin, Assigned: evilpie)

Tracking

Trunk
mozilla2.0b9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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.

Comment 1

7 years ago
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).)

Updated

7 years ago
Blocks: 578225
(Assignee)

Comment 2

7 years ago
Created attachment 494397 [details] [diff] [review]
PIC for string object length

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

Updated

7 years ago
Attachment #494397 - Flags: review?(dvander)

Comment 3

7 years ago
(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)
(Assignee)

Comment 5

7 years ago
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.
(Assignee)

Comment 9

7 years ago
Created attachment 495072 [details] [diff] [review]
PIC for string object length v2
(Assignee)

Updated

7 years ago
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+
(Assignee)

Comment 11

7 years ago
Created attachment 500287 [details]
PIC for string object length
Assignee: general → evilpies
Attachment #494397 - Attachment is obsolete: true
Attachment #495072 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #500287 - Flags: review+
(Assignee)

Comment 12

7 years ago
Created attachment 500288 [details] [diff] [review]
PIC for object wrapped string length
Attachment #500287 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
Created attachment 500289 [details] [diff] [review]
PIC for object-wrapped string length

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
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 623474
You need to log in before you can comment on or make changes to this bug.