fine-tune new iterator code

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

({perf, student-project})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

8 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:

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

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

Updated

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

/be

Comment 3

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

/be

Comment 5

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

Comment 6

8 years ago
Ideal starter project for a JS intern.
(Assignee)

Comment 7

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

Comment 8

7 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

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

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

/be
(Assignee)

Comment 11

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