1339535 - JSObject::setHadElementsAccess happens too early

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Tom S [:evilpie]]

Calling JSObject::setHadElementsAccess in SetObjectElementOperation always forces a new shape. This commonly prevents us from attaching CacheIR addSlot stubs. The flag HAD_ELEMENTS_ACCESS however is only really considered for objects with more than 120 properties, so setting this flag really early is useless. I suggest we only try to set this flag when we have at least a few dozen elements, not with zero or one.

Comment 1

[Jan de Mooij [:jandem]]

And we shouldn't do this for symbol properties, as object[symbol] is the only way to add symbol props so we can't assume it's a hashamp (and a hashmap with only symbol keys would be pretty weird). This flag is just a hack for sunspider unpack-code and the like.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

This uses MAX_HEIGHT_WITH_ELEMENTS_ACCESS / 3 (= 42). It fixes the regression in bug 1341067 comment 6. We can tweak this more later but this will avoid setting the flag in a lot of cases.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a9b7fb94f2c
Set the hadElementsAccess flag less eagerly to avoid unnecessary Shape changes. r=evilpie

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a9b7fb94f2c

Comment 5

[(no longer active)]

Jan, would you have expected this to help with Speedometer? (see bug 1339557 comment 3)

Comment 6

[Jan de Mooij [:jandem]]

(In reply to :Ehsan Akhgari from comment #5)
> Jan, would you have expected this to help with Speedometer? (see bug 1339557
> comment 3)

It helped Speedometer but I think most of that was fixing the regression from bug 1341067. This bug was mostly an issue for Baseline and after bug 1341067 it also affected Ion.

35% improvement is unlikely though, I'm not seeing that locally or on AWFY.