879393 - slow js - firefox: 730ms; chrome: 240ms (3x)

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Leon Sorokin]

Attachment: testcase

Wrote attached histogram code, then ran it through FF trunk and current stable Chrome. Resulting timings were:

firefx: 730ms
chrome: 240ms

See labeled slow section @ line 46, see console for loop iteration timings.

There's not too much happening there (typed array access, hash check by key + increment).

thanks!

Comment 1

[:Gijs (away/busy until Mar 10)]

Ugh, did not mean to switch components. Bad bugzilla.

Comment 2

[:Gijs (away/busy until Mar 10)]

Comment on attachment 758073 [details]
testcase

Relevant code snippets:

		// iterates @bbox within a parent rect of width
		// @wid; calls @fn, passing index within parent
		function iterBox(bbox, wid, fn) {
			var b = bbox,
				i0 = b.y * wid + b.x,
				i1 = (b.y + b.h - 1) * wid + (b.x + b.w -1),
				cnt = 0, incr = wid - b.w + 1, i = i0;

			do {
				fn.call(this, i);
				i += (++cnt % b.w == 0) ? incr : 1;
			} while (i <= i1);
		}

called with:

				function(i) {
					col = buf32[i];

					if (gpal[col])
						return;
					else if (lpal[col])
						lpal[col]++;
					else
						lpal[col] = 1;

				}

Is this just as slow if you inline the function?

Comment 3

[:Gijs (away/busy until Mar 10)]

It'd also be useful if you let us know which version(s) of Firefox and Chrome you checked with, and/or if you can check with a recent (ie nightly) build: http://nightly.mozilla.org/

Thanks!

Comment 4

[Leon Sorokin]

tested in latest nightly ff and current stable chrome, 27 i think.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Jit inspector shows some slow-looking opcodes in that innermost loop: [TestVAndBranch:MightEmulateUndefined], [LoadTypedArrayElementHole], lots of [TypeBarrier]...

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Shell testcase

Warning: large JS file when unzipped due to image data array.


Leon, while creating this I noticed there's probably a bug on this line:

boxes = makeBoxes(ctx.canvas.width, ctx.canvas.width, boxW, boxH),

The second one should be ctx.canvas.height. Fixing that makes both SM and V8 a good bit faster but we're still slower. Investigating.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Shell testcase v2

Bleh, my shell testcase also had a bug.

Numbers:

SM: 322 ms
d8: 106 ms

After changing the second imgW to imgH here: makeBoxes(imgW, imgW, boxW, boxH):

SM:  81 ms
d8:  13 ms

So the main problem here is out-of-bounds array accesses, but I will see where the remaining difference comes from.

Comment 8

[Jan de Mooij [:jandem]]

We read a double out of an Uint32Array, then when we use that value as index (gpal[col]) we assume it's an int32 and the DoubleToInt32 bails out:

col = buf32[i];

if (gpal[col])
    return;

We should fix GETELEM/SETELEM to not convert doubles to int32 in this case, maybe using Baseline IC type information. Also, I don't think we have fast paths when the index is a double, that also should be fixed.

Comment 9

[Leon Sorokin]

yeah sorry, i noticed that width bug about 10 min after i upped it and forgot to mention that here or upl a fix :(

the upside being that now there's more to optimize :)

Comment 10

[Leon Sorokin]

something else i noticed that's related to type-conversion of hash keys is the behavior of the following:

for (var key in {7777: 1, 225: 1})
    console.log(key);

// log: "7777", "225"

for (var key in {666: 1, 225: 1})
    console.log(key);

// log: "225", "666"

in chrome, the elements always come back ordered numerically. in my case, i actually prefer them to iterate in the originally specified order, but realize this is not guaranteed and cannot be relied upon.

to retain the order what i end up doing is "+" + <int32> (turning it into a string) when using it as a hash key, this way the order is preserved in both browsers. when reading the keys back i do an implicit conversion using +key if needed. hopefully the proposed changes do not affect hashes that are all string-keyd and the implicit conversion doesnt begin to interpret "+<int32>" as a number.

Comment 11

[Till Schneidereit [:till]]

(In reply to Leon Sorokin from comment #10)
> to retain the order what i end up doing is "+" + <int32> (turning it into a
> string) when using it as a hash key, this way the order is preserved in both
> browsers. when reading the keys back i do an implicit conversion using +key
> if needed. hopefully the proposed changes do not affect hashes that are all
> string-keyd and the implicit conversion doesnt begin to interpret "+<int32>"
> as a number.

A string starting with a "+" won't ever be interpreted as a number. However, you shouldn't rely on that, even now. Iteration order over objects is explicitly undefined, so your code could break with every minor release of every browser. If you need an ordered collection, use an array or a linked list.

Comment 12

[Till Schneidereit [:till]]

Current numbers for shell test 2 (best of ten runs):

SpiderMonkey end of December: 128ms
v8 3.21.17:                    91ms
jsc (current Safari):         177ms

These numbers are very different from jandem's in comment 8. Don't know what that's about.

Anway, Jandem, is the fix from comment 8 still on your radar?

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Till Schneidereit [:till] from comment #12)
> Anway, Jandem, is the fix from comment 8 still on your radar?

Not right now; maybe I'll have time to get to this in a few weeks.