Bug 4124 – change JavaScript property access to avoid double lookup

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

[Maciej Stachowiak]

JavaScript variable access currently looks up most variables twice. Related to this is the fact that 
hasOwnProperty and get effectively duplicate some functionality and need to be kept in sync. It would be 
nice to clean this up.

[Maciej Stachowiak]

Created attachment 3077 [details]
property change patch - changes get() to getOwnProperty()

[Geoffrey Garen]

+      if (!v || v == UndefinedImp::staticUndefined)
+        return false;

Our code seems to disagree about whether to use "X.type() == UndefinedType", "X.isUndefined()", or "X== 
UndefinedImp::staticUndefined". Is there a Good Reason (TM) for this, or should we standardize it?

[Maciej Stachowiak]

It happens that since the undefined value is unique, then so long as there is an interpreter in existence, all 
three will mean the same thing. However, comparing for == UndefinedImp::staticUndefined is likely faster 
than doing the type check, since it is a simple pointer compare rather than a virtual function call.

So probably the best option would be to make isUndefined use the comparison to staticUndefined, and 
then use the isUndefined call everywhere for the sake of abstraction.

In this case I used the "compare to staticUndefined" technique because it's what the code already did, and 
not for any real reason.

[Darin Adler]

The patch I have sitting in my tree does exactly what Maciej suggests among many other optimizations. It 
changes all checks for Undefined to use isUndefined() and makes it use the fastest idiom (which seems to 
be comparing with the global singleton instance).

[Darin Adler]

Comment on attachment 3077 [details]
property change patch - changes get() to getOwnProperty()

Since Value already has a distinct "null" value, why didn't you use that
instead of adding a new result parameter?

The reason I ask is that once I change Value to ValueImp, it because quite
natural to have NULL mean "I don't have a property" even though it's in-band
signalling. The change would be smaller if getOwnProperty simply returned
Value() when there is no property, which is what getDirect does.

I also think this would result in a patch with many fewer lines changed.

Otherwise this patch looks great to me.

By the way, my pending patch is going to get us a bunch *more* speedup on top
of this, so I'm pretty excited about the combination.

[Maciej Stachowiak]

I considered and rejected the in-band signalling option, because it wouldn't fit very well with the eventual 
plan to eliminate hasOwnProperty entirely. In that case you definitely need separate returns for presence 
of a property and its actual value. And this will be even more the case with the eventual property slot 
design. We can make access and simple assignment efficient without property slots but they are still 
needed to make update operations like "++" and "+=" efficient, and these are quite common both in 
benchmarks and in real-world web pages.

[Darin Adler]

Comment on attachment 3077 [details]
property change patch - changes get() to getOwnProperty()

Maciej convinced me that as a first step, this use of bool is superior to the
in-band signaling of using Value(), given what we plan next.

I re-reviewed the entire patch. r=me

[Darin Adler]

Maciej landed this today.