Bug 4124 - change JavaScript property access to avoid double lookup
Summary: change JavaScript property access to avoid double lookup
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Maciej Stachowiak
Depends on:
Reported: 2005-07-24 22:46 PDT by Maciej Stachowiak
Modified: 2005-07-25 20:35 PDT (History)
1 user (show)

See Also:

property change patch - changes get() to getOwnProperty() (203.36 KB, patch)
2005-07-24 23:28 PDT, Maciej Stachowiak
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Maciej Stachowiak 2005-07-24 22:46:09 PDT
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.
Comment 1 Maciej Stachowiak 2005-07-24 23:28:54 PDT
Created attachment 3077 [details]
property change patch - changes get() to getOwnProperty()
Comment 2 Geoffrey Garen 2005-07-25 01:30:45 PDT
+      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?
Comment 3 Maciej Stachowiak 2005-07-25 02:17:41 PDT
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.
Comment 4 Darin Adler 2005-07-25 10:13:47 PDT
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).
Comment 5 Darin Adler 2005-07-25 12:06:30 PDT
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.
Comment 6 Maciej Stachowiak 2005-07-25 14:40:31 PDT
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.
Comment 7 Darin Adler 2005-07-25 15:02:17 PDT
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
Comment 8 Darin Adler 2005-07-25 20:35:34 PDT
Maciej landed this today.