Bug 133558 - Eagerly reify DOM prototype attributes
Summary: Eagerly reify DOM prototype attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-05 13:25 PDT by Mark Hahnenberg
Modified: 2014-06-18 03:49 PDT (History)
9 users (show)

See Also:


Attachments
Patch (57.45 KB, patch)
2014-06-05 13:36 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (57.45 KB, patch)
2014-06-05 13:43 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (59.12 KB, patch)
2014-06-05 14:01 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (59.56 KB, patch)
2014-06-05 14:23 PDT, Mark Hahnenberg
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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