fine-tune new iterator code




9 years ago
8 years ago


(Reporter: njn, Assigned: njn)


({perf, student-project})

Firefox Tracking Flags

(Not tracked)



(2 attachments)



9 years ago
Created attachment 445627 [details]
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:

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

9 years ago
Awesome nick. Thanks a lot for looking at this.


9 years ago
Keywords: perf, student-project
Who is calling js_GetProperty?


Comment 3

9 years ago
I didn't look but this is likely the GetPropertyByName path from trace.
(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.


Comment 5

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

Comment 6

9 years ago
Ideal starter project for a JS intern.

Comment 7

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

Comment 8

9 years ago
Created attachment 475886 [details] [diff] [review]
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.
Attachment #475886 - Flags: review?(gal)

Comment 9

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

Attachment #475886 - Flags: review?(gal) → review+
I wonder whether MSVC managed to fuse the common tails without this patch.


Comment 11

8 years ago
This change was subsumed by the patch from bug 606573.
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.