Bug 133558

Summary: Eagerly reify DOM prototype attributes
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, cdumez, cgarcia, clopez, commit-queue, gyuyoung.kim, kling, rakuco, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch oliver: review+

Description Mark Hahnenberg 2014-06-05 13:25:36 PDT
This allows us to get rid of a lot of the additional overhead of pushing DOM attributes up into the prototype. By eagerly reifying the custom getters and setters into the actual JSObject we avoid having to override getOwnPropertySlot for all of the DOM prototypes, which is a lot of the overhead of doing property lookups on DOM wrappers.
Comment 1 Mark Hahnenberg 2014-06-05 13:36:30 PDT
Created attachment 232580 [details]
Patch
Comment 2 WebKit Commit Bot 2014-06-05 13:39:19 PDT
Attachment 232580 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CustomGetterSetter.h:72:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Hahnenberg 2014-06-05 13:43:35 PDT
Created attachment 232581 [details]
Patch
Comment 4 WebKit Commit Bot 2014-06-05 13:46:26 PDT
Attachment 232581 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CustomGetterSetter.h:72:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Mark Hahnenberg 2014-06-05 14:01:53 PDT
Created attachment 232582 [details]
Patch
Comment 6 WebKit Commit Bot 2014-06-05 14:03:29 PDT
Attachment 232582 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CustomGetterSetter.h:72:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Mark Hahnenberg 2014-06-05 14:23:39 PDT
Created attachment 232583 [details]
Patch
Comment 8 WebKit Commit Bot 2014-06-05 14:24:35 PDT
Attachment 232583 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/CustomGetterSetter.h:72:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Oliver Hunt 2014-06-05 18:38:19 PDT
Comment on attachment 232583 [details]
Patch

You'll need to do run-bindings-tests verify that the diffs look sane, then run-bindings-tests --reset-results otherwise yay!
Comment 10 Andreas Kling 2014-06-07 10:04:45 PDT
This doesn't work for HTMLDListElement and HTMLOListElement. They crap out in InterfaceRequiresAttributesOnInstance because they have "List" in the name. If you change that line to something like this instead:

    return 1 if $interfaceName =~ /List$/;

..then this will go even faster, as the jslib test pages have a bunch of these elements. :)

Other than that, truly awesome stuff.
Comment 11 Mark Hahnenberg 2014-06-09 11:54:08 PDT
Committed r169703: <http://trac.webkit.org/changeset/169703>
Comment 12 Mark Hahnenberg 2014-06-09 12:31:59 PDT
Followup fixes in http://trac.webkit.org/changeset/169705 and http://trac.webkit.org/changeset/169707
Comment 13 Alexey Proskuryakov 2014-06-09 15:31:29 PDT
js/dom/global-constructors-attributes-dedicated-worker.html started to crash every time in debug builds after this. Mark is working on a fix.

http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=js%2Fdom%2Fglobal-constructors-attributes-dedicated-worker.html
Comment 14 Mark Hahnenberg 2014-06-09 15:39:04 PDT
The regression was actually a pre-existing bug that this change revealed, so I filed bug 133661 to track it separately.
Comment 15 Carlos Alberto Lopez Perez 2014-06-18 03:49:42 PDT
(In reply to comment #11)
> Committed r169703: <http://trac.webkit.org/changeset/169703>

This caused a regression of ~50% on memory usage (JSHeap) on all performance tests: https://bugs.webkit.org/show_bug.cgi?id=133876