Bug 131579 - Make NodeList.length inline-cacheable by JSC.
Summary: Make NodeList.length inline-cacheable by JSC.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-11 19:26 PDT by Andreas Kling
Modified: 2014-04-11 23:00 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.06 KB, patch)
2014-04-11 19:29 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (1.63 KB, patch)
2014-04-11 19:30 PDT, Andreas Kling
ggaren: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (514.39 KB, application/zip)
2014-04-11 20:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (536.97 KB, application/zip)
2014-04-11 21:08 PDT, Build Bot
no flags Details
Patch (2.04 KB, patch)
2014-04-11 21:33 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (2.86 KB, patch)
2014-04-11 21:35 PDT, Andreas Kling
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2014-04-11 19:26:48 PDT
NodeList.length currently isn't inline-cacheable because CodeGeneratorJS.pm makes everything that has and index or named getter uncacheable.
I don't think we need to be doing that.
Comment 1 Andreas Kling 2014-04-11 19:29:43 PDT
Created attachment 229193 [details]
Patch
Comment 2 Andreas Kling 2014-04-11 19:30:18 PDT
Created attachment 229194 [details]
Patch
Comment 3 Geoffrey Garen 2014-04-11 19:47:24 PDT
Comment on attachment 229194 [details]
Patch

r=me
Comment 4 Build Bot 2014-04-11 20:33:59 PDT
Comment on attachment 229194 [details]
Patch

Attachment 229194 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5534621794041856

New failing tests:
fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml
Comment 5 Build Bot 2014-04-11 20:34:04 PDT
Created attachment 229195 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 6 Build Bot 2014-04-11 21:08:40 PDT
Comment on attachment 229194 [details]
Patch

Attachment 229194 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6509598697586688

New failing tests:
fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml
Comment 7 Build Bot 2014-04-11 21:08:48 PDT
Created attachment 229197 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 8 Andreas Kling 2014-04-11 21:22:01 PDT
Comment on attachment 229194 [details]
Patch

I have a cleaner fix.
Comment 9 Andreas Kling 2014-04-11 21:33:56 PDT
Created attachment 229198 [details]
Patch

Better patch: just mark the slot cacheable when we find a builtin that takes precedence over a named/indexed item.
Comment 10 Andreas Kling 2014-04-11 21:35:37 PDT
Created attachment 229199 [details]
Patch

(With updated bindings test results this time.)
Comment 11 Oliver Hunt 2014-04-11 21:39:16 PDT
Comment on attachment 229199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229199&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:407
>              push(@getOwnPropertySlotImpl, "    const ${namespaceMaybe}HashTableValue* entry = getStaticValueSlotEntryWithoutCaching<$className>(exec, propertyName);\n");

Do  we still want getStaticValueSlotEntryWithoutCaching?
Comment 12 Andreas Kling 2014-04-11 21:46:29 PDT
Comment on attachment 229199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229199&action=review

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:407
>>              push(@getOwnPropertySlotImpl, "    const ${namespaceMaybe}HashTableValue* entry = getStaticValueSlotEntryWithoutCaching<$className>(exec, propertyName);\n");
> 
> Do  we still want getStaticValueSlotEntryWithoutCaching?

I think so. Plain getStaticValueSlot will fall back to the parent's getOwnPropertySlot, that's not what we want right?
Comment 13 Oliver Hunt 2014-04-11 22:14:30 PDT
(In reply to comment #12)
> (From update of attachment 229199 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229199&action=review
> 
> >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:407
> >>              push(@getOwnPropertySlotImpl, "    const ${namespaceMaybe}HashTableValue* entry = getStaticValueSlotEntryWithoutCaching<$className>(exec, propertyName);\n");
> > 
> > Do  we still want getStaticValueSlotEntryWithoutCaching?
> 
> I think so. Plain getStaticValueSlot will fall back to the parent's getOwnPropertySlot, that's not what we want right?

Haha, the sole point of getStaticValueSlotEntryWithoutCaching is to get the property getter (and setter?) without taking a property slot and doing setCacheableCustom
Comment 14 Andreas Kling 2014-04-11 23:00:07 PDT
Committed r167181: <http://trac.webkit.org/changeset/167181>