Bug 74331 - De-virtualize destructors
Summary: De-virtualize destructors
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: 74332 74333 74335 74336 74338 74339 74341 74342 74347 74348 74349 74350 74353 74355
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-12 14:21 PST by Mark Hahnenberg
Modified: 2011-12-16 11:22 PST (History)
6 users (show)

See Also:


Attachments
Patch (278.16 KB, patch)
2011-12-13 15:02 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (282.01 KB, patch)
2011-12-13 17:02 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (302.79 KB, patch)
2011-12-14 11:28 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (303.14 KB, patch)
2011-12-14 16:11 PST, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (302.96 KB, patch)
2011-12-14 21:27 PST, Mark Hahnenberg
ggaren: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Bencher results (7.03 KB, text/plain)
2011-12-16 11:22 PST, Mark Hahnenberg
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2011-12-12 14:21:25 PST
In order to remove the virtual destructors, which are the last of the virtual functions, from the JSCell hierarchy, we need to add the ClassInfo pointer to the cell rather than to the structure because in order to be able to lazily call the static destroy() functions that will replace the virtual destructors, we need to be able to access the ClassInfo without the danger of the object's Structure being collected before the object itself.

This is a tracking bug for that effort.  Each of the patches will be put up individually for review, but they will all be landed together as one giant patch to avoid any performance regressions (I also only verified that each patch would build on 64-bit Mac, and later went back and stabilized the other platforms).
Comment 1 Mark Hahnenberg 2011-12-13 15:02:46 PST
Created attachment 119091 [details]
Patch
Comment 2 Mark Hahnenberg 2011-12-13 17:02:17 PST
Created attachment 119112 [details]
Patch
Comment 3 Mark Hahnenberg 2011-12-13 17:04:59 PST
Comment on attachment 119112 [details]
Patch

This patch is obviously more than just de-virtualizing destructors.  Destructors were the last virtual function in the JSCell hierarchy, so their removal caused a cascade of other necessary changes to be done in the same patch.
Comment 4 WebKit Review Bot 2011-12-13 17:08:26 PST
Attachment 119112 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/API/JS..." exit_code: 1

Source/JavaScriptCore/jit/JITOpcodes.cpp:52:  string_failureCases2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:55:  string_failureCases2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 144 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Early Warning System Bot 2011-12-13 17:41:20 PST
Comment on attachment 119112 [details]
Patch

Attachment 119112 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10876038
Comment 6 Gyuyoung Kim 2011-12-13 18:06:54 PST
Comment on attachment 119112 [details]
Patch

Attachment 119112 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10876045
Comment 7 Gustavo Noronha (kov) 2011-12-13 18:54:21 PST
Comment on attachment 119112 [details]
Patch

Attachment 119112 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10873050
Comment 8 Collabora GTK+ EWS bot 2011-12-13 23:11:39 PST
Comment on attachment 119112 [details]
Patch

Attachment 119112 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10875113
Comment 9 Mark Hahnenberg 2011-12-14 11:28:56 PST
Created attachment 119263 [details]
Patch
Comment 10 WebKit Review Bot 2011-12-14 11:32:27 PST
Attachment 119263 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/API/JS..." exit_code: 1

Source/JavaScriptCore/jit/JITOpcodes.cpp:52:  string_failureCases2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:55:  string_failureCases2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 152 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Geoffrey Garen 2011-12-14 11:39:07 PST
Comment on attachment 119263 [details]
Patch

Since I've already reviewed the sub-patches, r=me if it builds.
Comment 12 Early Warning System Bot 2011-12-14 11:45:54 PST
Comment on attachment 119263 [details]
Patch

Attachment 119263 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10872346
Comment 13 Gyuyoung Kim 2011-12-14 12:20:46 PST
Comment on attachment 119263 [details]
Patch

Attachment 119263 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10872357
Comment 14 Gustavo Noronha (kov) 2011-12-14 13:08:42 PST
Comment on attachment 119263 [details]
Patch

Attachment 119263 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10873360
Comment 15 Gustavo Noronha (kov) 2011-12-14 15:38:10 PST
Comment on attachment 119263 [details]
Patch

Attachment 119263 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10870456
Comment 16 Mark Hahnenberg 2011-12-14 16:11:51 PST
Created attachment 119317 [details]
Patch
Comment 17 WebKit Review Bot 2011-12-14 16:16:44 PST
Attachment 119317 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/API/JS..." exit_code: 1

Source/JavaScriptCore/jit/JITOpcodes.cpp:52:  string_failureCases2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:55:  string_failureCases2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 152 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Gyuyoung Kim 2011-12-14 16:56:00 PST
Comment on attachment 119317 [details]
Patch

Attachment 119317 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10905021
Comment 19 Mark Hahnenberg 2011-12-14 21:27:56 PST
Created attachment 119371 [details]
Patch
Comment 20 WebKit Review Bot 2011-12-14 21:30:30 PST
Attachment 119371 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/API/JS..." exit_code: 1

Source/JavaScriptCore/jit/JITOpcodes.cpp:52:  string_failureCases2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:55:  string_failureCases2 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 152 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Geoffrey Garen 2011-12-15 18:47:11 PST
Comment on attachment 119371 [details]
Patch

r=me if it builds.
Comment 22 WebKit Review Bot 2011-12-15 20:21:20 PST
Comment on attachment 119371 [details]
Patch

Rejecting attachment 119371 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
/Netscape/JSNPMethod.cpp
patching file Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.cpp
patching file Source/WebKit2/WebProcess/Plugins/Netscape/JSNPObject.h
patching file Source/WebKit2/win/WebKit2.def
patching file Source/WebKit2/win/WebKit2CFLite.def
patching file Source/autotools/symbols.filter
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Geoffrey Garen', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/10914511
Comment 23 Mark Hahnenberg 2011-12-16 11:13:08 PST
Committed r103083: <http://trac.webkit.org/changeset/103083>
Comment 24 Mark Hahnenberg 2011-12-16 11:22:54 PST
Created attachment 119639 [details]
Bencher results

Performance results for this patch.  Mostly a wash, small win on kraken.