1060359 - Bad type information for COW array with mixed element type

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

I ran into this while writing a micro-benchmark for bug 1060342. Below is a testcase where we are now using a GetElementCache instead of LoadElement, because |arr| has type AnyObject. If I change the array literal to [1, 2] we get a LoadElement.

function f() {
    var res = 0;
    var arr = [1, "foo"];
    for (var i=0; i<1000000000; i++) {
	res = arr[i % 2];
    }
    return res;
}
f();

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

I think before COW arrays we would be using a cache here too; the problem is that when an array literal doesn't have a uniform element type we give it the generic 'new' type for Array.prototype, which has unknown properties.  With COW arrays though this has changed, since the COW array has a type tied to the initialization site.  We were never giving the array that type in this case though, since the unknown properties confused GetOrFixupCopyOnWriteObject.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8483039 [details] [diff] [review]
patch

Review of attachment 8483039 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Hackett (:bhackett) from comment #1)
> I think before COW arrays we would be using a cache here too; the problem is
> that when an array literal doesn't have a uniform element type we give it
> the generic 'new' type for Array.prototype, which has unknown properties.

Are you sure? I don't get the cache if I change [1, "foo"] (COW) to [1, "foo".toString()] (non-COW).

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Comment on attachment 8483039 [details] [diff] [review]
> patch
> 
> Review of attachment 8483039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Brian Hackett (:bhackett) from comment #1)
> > I think before COW arrays we would be using a cache here too; the problem is
> > that when an array literal doesn't have a uniform element type we give it
> > the generic 'new' type for Array.prototype, which has unknown properties.
> 
> Are you sure? I don't get the cache if I change [1, "foo"] (COW) to [1,
> "foo".toString()] (non-COW).

Oops, yeah, you're right.  I was thinking about array literals with all constants, for which FixArrayType() will produce an array with unknown type information if the constants don't have consistent type.  But before COW arrays we didn't use FixArrayType() on such arrays when they're in code that can run multiple times (we still use it on e.g. JSON arrays) and it is only called incidentally for COW arrays.

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecde022a7e52

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ecde022a7e52