566229 - fine-tune new iterator code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: Cachegrind results

Andreas said:

> There is a bunch of sharking and fine-tuning left for iteration
> (ObjectToIterator, CloseIterator). The wins from the current patch are
> mostly algorithmic. I didn't even look at it i shark yet. I have a couple
> other higher priority stuff on my list (proxies, cycle collection), so if
> anyone wants to jump on it, remember its always ok to steal from me ;)
> Otherwise I will get back on this in a bit.

I've attached a Cachegrind profile of string-fasta, which exercises iterators heavily.  One thing I noticed:

JSBool
js_GetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
{
    return js_GetPropertyHelper(cx, obj, id, JSGET_METHOD_BARRIER, vp);
}

The call to js_GetPropertyHelper() isn't inlined, which costs 1.7M instructions.

Also, it's possible to squeeze out another 1M instructions by replacing the first call to GetIterator() in js_ValueToIterator() with a goto that jumps to the second call;  this helps (on 32-bit Linux, at least) because it allows GetIterator() to be inlined.

Comment 1

[Andreas Gal :gal]

Awesome nick. Thanks a lot for looking at this.

Comment 2

[Brendan Eich [:brendan]]

Who is calling js_GetProperty?

/be

Comment 3

[Andreas Gal :gal]

I didn't look but this is likely the GetPropertyByName path from trace.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> I didn't look but this is likely the GetPropertyByName path from trace.

Sure, but don't you have a bug on using an induction variable, essentially (the sprop) to eliminate this altogether? We also discussed using a PIC; dmandelin is hip to this.

/be

Comment 5

[Andreas Gal :gal]

js_GetProperty should still be moved under the helper so it can inline.

Comment 6

[Andreas Gal :gal]

Ideal starter project for a JS intern.

Comment 7

[Nicholas Nethercote [inactive]]

Bug 593931, bug 596026, and bug 596029 cover js_GetProperty().  That just leaves the GetIterator() case.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: patch (against TM 53617:c123e94f7737)

Use a goto so that GetIterator() is only called once in js_ValueToIterator().  Saves 2 million instructions (out of 2 billion) on SunSpider, mostly in string-fasta -- a small win, but it's a small patch.

Comment 9

[Andreas Gal :gal]

Comment on attachment 475886 [details] [diff] [review]
patch (against TM 53617:c123e94f7737)

++njn

Comment 10

[Brendan Eich [:brendan]]

I wonder whether MSVC managed to fuse the common tails without this patch.

/be

Comment 11

[Nicholas Nethercote [inactive]]

This change was subsumed by the patch from bug 606573.