Bug 6083 – REGRESSION: 5-10% performance regression on JS iBench from getter/setter change

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Maciej Stachowiak]

On my machine, the getter/setter change caused a regression in JavaScript iBench speed from 1980ms or 
so to ~2150ms. Anders and I have discussed possible ways to mitigate the performance impact.

[Darin Adler]

I can't believe I didn't notice the use of the virtual function type() in the very hot get function.

One possibility is setting a special bit pattern in the low two bits of the GetterSetterImp* ala the 
01 tag mask used in SimpleNumber. Then you can check if a property value is a GetterSetterImp* 
without dispatching to type().

Unfortunately, that might slow down garbage collection since the mark function in the property 
map would have to know about this convention.

Another possibility is to not have the GetterSetterImp be garbage collected at all, since they are 
uniquely owned by a particular property slot in a property hash table. So we can teach the 
property hash table about some special pointer convention and give them sole ownership.

I think with this suggested design we'd have only a single mask and branch in the hot 
getOwnPropertySlot function so we'd probably be "back in business".

[Anders Carlsson]

Created attachment 5117 [details]
Speed patch

For the record, here's a patch that me and Maciej have been working on. It adds
a GetterSetter attribute which getOwnPropertySlot can examine to see if the
slot needs to be a getter slot.

It also adds a hasSetter flag to the property map of each object, which should
speed up ::put a bit since you only need to check the flag and not do a hash
table lookup for each call to ::put.

Another thing I've been considering is to remove the originalObject argument to
PropertySlot::GetValueFunc; it's only used in one function,
PropertySlot::functionGetter. Since functionGetter consists of just invokeing
the call function, I think we could simply just fold that into
PropertySlot::getValue and set getValue to a special value, like setValueSlot
does. That would save us an extra argument to all calls to all getter
functions. (Not sure whether this would help performance, but it sure feels
like it)

[Maciej Stachowiak]

Created attachment 5178 [details]
latest patch

Here's the latest patch I had, much of it is irrelevant or wrong but what is
surprising is that it still causes a slowdown, solely due to the
getOwnPropertySlot changes (put changes make no difference any more, nor does
the extra argument to getValue as far as I can tell). The problem is that
JSObject::getOwnPropertSlot no longer gets inlined into derived classes calling
the base class, most notably ActivationImp. Even though I have it marked
always_inline! Not sure why.

Some of the fixes here are for warnings from -Winline.

[Maciej Stachowiak]

I like Darin's suggestions but I think my slightly simpler patch would work if only it would inline - even 
with most of the bad stuff commented it refuses to.

[Maciej Stachowiak]

Created attachment 5282 [details]
this one gets the regression down to ~1% and doesn't break anything afaik

[Darin Adler]

Comment on attachment 5282 [details]
this one gets the regression down to ~1% and doesn't break anything afaik

JSObject::getOwnPropertySlot should use the ALWAYS_INLINE macro rather than
doing it directly.

Why do the hasGetterSetterProperties() and setHasGetterSetterProperties()
functions have commented out code instead of the real code?

Otherwise, looks good. r=me

[Geoffrey Garen]

Is there a reason to use a hasGetterSetter flag instead of a numGetterSetters count on the property map? 
Using a count would eliminate the loop in deleteProperty.

[Maciej Stachowiak]

OK, I fixed the problems Darin pointed out (d'oh). Now the patch leaves about a 2% regression (2020ms 
times vs 1980 from before). JSObject::getOwnPropertySlot is once again not getting inlined, despite 
ALWAYS_INLINE declaration. Maybe someone can help me figure out why! I will make a separate bug for 
the remaining regression.