557353 - JM: PIC for object-wrapped string length

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Mandelin [:dmandelin]]

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

[Luke Wagner [:luke]]

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

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: 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.

Comment 3

[Luke Wagner [:luke]]

(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 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 5

[Tom S. (please needinfo tschuster)]

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.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

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 ;)

Comment 7

[Brendan Eich [:brendan]]

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

Comment 8

[Brian Hackett [Laid off!]]

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

Comment 9

[Tom S. (please needinfo tschuster)]

Attachment: PIC for string object length v2

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 11

[Tom S. (please needinfo tschuster)]

Attachment: PIC for string object length

Comment 12

[Tom S. (please needinfo tschuster)]

Attachment: PIC for object wrapped string length

Comment 13

[Tom S. (please needinfo tschuster)]

Attachment: PIC for object-wrapped string length

Sorry for spam

Comment 14

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/feb28ec64b74

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/feb28ec64b74