Bug 28664 - Array index miss case creates a string every time
Summary: Array index miss case creates a string every time
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-22 23:33 PDT by Darin Adler
Modified: 2009-08-24 14:38 PDT (History)
1 user (show)

See Also:


Attachments
patch (3.67 KB, patch)
2009-08-22 23:36 PDT, Darin Adler
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2009-08-22 23:33:26 PDT
The object prototype is creating a string and searching for it in the Identifier table every time there's a miss. This is bad on the crypto tests in SunSpider, and in other real-world code too I'm sure.
Comment 1 Darin Adler 2009-08-22 23:36:59 PDT
Created attachment 38446 [details]
patch
Comment 2 Darin Adler 2009-08-22 23:45:46 PDT
Comment on attachment 38446 [details]
patch

Maybe it should be called m_mayHaveIntegralNamedProperty because I don't have any code to set it to false if the offending properties are all removed.
Comment 3 Darin Adler 2009-08-22 23:46:55 PDT
Comment on attachment 38446 [details]
patch

Or I could reverse its sense and call this m_hasNoIntegralNamedProperties.
Comment 4 David Kilzer (:ddkilzer) 2009-08-23 05:55:13 PDT
Comment on attachment 38446 [details]
patch

>+    if (isUInt32)
>+        m_hasIntegralNamedProperty = true;

You could save a branch here by always assigning m_hasIntegralNamedProperty:

    m_hasIntegralNamedProperty = isUInt32;

Not sure if that's helpful or not, though.
Comment 5 Darin Adler 2009-08-23 14:07:30 PDT
(In reply to comment #4)
> (From update of attachment 38446 [details])
> >+    if (isUInt32)
> >+        m_hasIntegralNamedProperty = true;
> 
> You could save a branch here by always assigning m_hasIntegralNamedProperty:
> 
>     m_hasIntegralNamedProperty = isUInt32;
> 
> Not sure if that's helpful or not, though.

That would break the case where isUInt32 is false and m_hasIntegralNamedProperty is already true, but I could do:

    m_hasIntegralNamedProperty |= isUInt32;

Or I could skip the entire check if m_hasIntegralNamedProperty is already true. No need to call toStrictUInt32 at all in that case.

But none of that seems important. We don't need to optimize adding properties to the object prototypes. We should optimize for clarity. My existing patch seems OK to me, but we could make changes.
Comment 6 Geoffrey Garen 2009-08-24 11:02:46 PDT
Comment on attachment 38446 [details]
patch

> +    return m_hasIntegralNamedProperty && JSObject::getOwnPropertySlot(exec, propertyName, slot);

I tend to think that code like this is more readable if the m_hasIntegralNamedProperty case is an explicit early return. Up to you.

r=me
Comment 7 Darin Adler 2009-08-24 14:35:36 PDT
http://trac.webkit.org/changeset/47727
Comment 8 Darin Adler 2009-08-24 14:38:00 PDT
http://trac.webkit.org/changeset/47730