Bug 68404 - Add static version of JSCell::visitChildren
Summary: Add static version of JSCell::visitChildren
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: 67690 68567
  Show dependency treegraph
 
Reported: 2011-09-19 15:39 PDT by Mark Hahnenberg
Modified: 2011-09-23 12:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch (99.82 KB, patch)
2011-09-20 16:14 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Resetting bindings (101.88 KB, patch)
2011-09-20 16:16 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fixing windows (115.46 KB, patch)
2011-09-21 11:35 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (113.42 KB, patch)
2011-09-21 11:56 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Benchmark results (4.43 KB, text/plain)
2011-09-21 12:56 PDT, Mark Hahnenberg
no flags Details
Fix GTK (115.76 KB, patch)
2011-09-21 18:09 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fix GTK for real (113.05 KB, patch)
2011-09-22 08:19 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fix bad merge (115.68 KB, patch)
2011-09-22 08:52 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (118.31 KB, patch)
2011-09-22 23:53 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (118.20 KB, patch)
2011-09-23 09:06 PDT, Mark Hahnenberg
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2011-09-19 15:39:11 PDT
In the ongoing effort to un-virtualize JSCell, we need to replace visitChildren with a non-virtual version.  This will eventually be done by adding a function pointer to the ClassInfo struct.

First we need to make a version of visitChildren that we can reference easily as a function pointer.  Since member methods are difficult to work into ClassInfo due to their implicit "this" and the special ways that compilers treat them, we will need to replace them with static methods.  Initially we'll add them and simply have the current virtual visitChildren call the static methods, which will in turn call their parent static method, etc.  This will preserve the correctness of the code while allowing us to work incrementally.
Comment 1 Mark Hahnenberg 2011-09-20 16:14:38 PDT
Created attachment 108072 [details]
Patch
Comment 2 Mark Hahnenberg 2011-09-20 16:16:20 PDT
Created attachment 108074 [details]
Resetting bindings
Comment 3 Darin Adler 2011-09-20 16:20:40 PDT
Comment on attachment 108074 [details]
Resetting bindings

I think that using the word static in the name of the new function is unclear and not helpful. It would be a shame to be left with that word in the names of functions after this task is done. If we have to rename something to keep these functions out of each others’ way, why not rename the virtual function that we are getting rid of? The new non-virtual version can be named visitChildren and the old could be called visitChildrenVirtual.

These new functions should be private, since they are not intended to be called directly outside of the class.
Comment 4 Mark Hahnenberg 2011-09-20 16:30:37 PDT
(In reply to comment #3)
> (From update of attachment 108074 [details])
> I think that using the word static in the name of the new function is unclear and not helpful. It would be a shame to be left with that word in the names of functions after this task is done. If we have to rename something to keep these functions out of each others’ way, why not rename the virtual function that we are getting rid of? The new non-virtual version can be named visitChildren and the old could be called visitChildrenVirtual.

In the end, my goal is to keep visitChildren in just JSCell so that any current call sites to the virtual version are unaffected.  There are a few instances of classes declaring internal visitChildrenDirect() methods.  Perhaps I can adopt that name for these new functions and merge any existing visitChildrenDirect() methods with the new ones?

> These new functions should be private, since they are not intended to be called directly outside of the class.

Some of the visitChildren functions call one another directly (especially children calling parent implementations) so at least some of them shouldn't be private.  I attempted to duplicate the access permissions of whatever the visitChildren function was before.  I can check for any instances that are incorrect, though.
Comment 5 Geoffrey Garen 2011-09-20 18:22:31 PDT
> In the end, my goal is to keep visitChildren in just JSCell so that any current call sites to the virtual version are unaffected.

I believe there is only one current caller of the virtual visitChildren: SlotVisitor::visitChildren(). (You can verify this by just making it private.) I think you should either remove that function entirely, or, like Darin suggested, give it the worse name (*Virtual).
Comment 6 Geoffrey Garen 2011-09-20 18:22:55 PDT
Comment on attachment 108074 [details]
Resetting bindings

r- for build failure and because I think the naming could be better.
Comment 7 Mark Hahnenberg 2011-09-21 11:35:17 PDT
Created attachment 108194 [details]
Fixing windows
Comment 8 Mark Hahnenberg 2011-09-21 11:56:44 PDT
Created attachment 108199 [details]
Patch
Comment 9 Geoffrey Garen 2011-09-21 12:32:29 PDT
Comment on attachment 108199 [details]
Patch

Looks good. r=me if it builds and is not a perf. regression.
Comment 10 Mark Hahnenberg 2011-09-21 12:56:27 PDT
Created attachment 108211 [details]
Benchmark results
Comment 11 Gustavo Noronha (kov) 2011-09-21 14:06:31 PDT
Comment on attachment 108199 [details]
Patch

Attachment 108199 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9770894
Comment 12 Mark Hahnenberg 2011-09-21 15:31:18 PDT
Not quite sure how to fix the build failure on GTK…link error with a method (JSNode::visitChildrenVirtual) that definitely exists (it's defined in WebCore/bindings/js/JSNodeCustom.cpp) and is exported in the WebCore.exp.in.  What linker magic does GTK need to get this to work?
Comment 13 Oliver Hunt 2011-09-21 15:48:02 PDT
Comment on attachment 108199 [details]
Patch

Looks like gtk-ews has gone weird
Comment 14 Martin Robinson 2011-09-21 16:27:29 PDT
(In reply to comment #12)
> Not quite sure how to fix the build failure on GTK…link error with a method (JSNode::visitChildrenVirtual) that definitely exists (it's defined in WebCore/bindings/js/JSNodeCustom.cpp) and is exported in the WebCore.exp.in.  What linker magic does GTK need to get this to work?

You need to add the symbol the the Source/autotools/symbols.filter to fix the GTK+ build.
Comment 15 Mark Hahnenberg 2011-09-21 18:09:18 PDT
Created attachment 108264 [details]
Fix GTK
Comment 16 Collabora GTK+ EWS bot 2011-09-21 21:00:12 PDT
Comment on attachment 108264 [details]
Fix GTK

Attachment 108264 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9794063
Comment 17 Gustavo Noronha (kov) 2011-09-21 21:11:59 PDT
Comment on attachment 108264 [details]
Fix GTK

Attachment 108264 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9793097
Comment 18 Geoffrey Garen 2011-09-21 21:59:42 PDT
Is the GTK build failure real? I can't tell from this output:

/usr/bin/ld:../../Source/autotools/symbols.filter:60: syntax error in VERSION script
Comment 19 Geoffrey Garen 2011-09-21 22:00:31 PDT
Comment on attachment 108264 [details]
Fix GTK

r=me, assuming the GTK build failure really is a syntax error in the VERSION script.
Comment 20 Martin Robinson 2011-09-21 23:17:06 PDT
(In reply to comment #18)
> Is the GTK build failure real? I can't tell from this output:
> 
> /usr/bin/ld:../../Source/autotools/symbols.filter:60: syntax error in VERSION script

Yes. The new line in symbols.filter is missing a semi-colon.
Comment 21 Mark Hahnenberg 2011-09-22 08:19:32 PDT
Created attachment 108335 [details]
Fix GTK for real
Comment 22 Gustavo Noronha (kov) 2011-09-22 08:29:01 PDT
Comment on attachment 108335 [details]
Fix GTK for real

Attachment 108335 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9792337
Comment 23 Early Warning System Bot 2011-09-22 08:32:36 PDT
Comment on attachment 108335 [details]
Fix GTK for real

Attachment 108335 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9794261
Comment 24 Gyuyoung Kim 2011-09-22 08:34:04 PDT
Comment on attachment 108335 [details]
Fix GTK for real

Attachment 108335 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9798236
Comment 25 Mark Hahnenberg 2011-09-22 08:52:01 PDT
Created attachment 108340 [details]
Fix bad merge
Comment 26 Geoffrey Garen 2011-09-22 09:45:54 PDT
Comment on attachment 108340 [details]
Fix bad merge

r=me, assuming it builds.
Comment 27 WebKit Review Bot 2011-09-22 18:57:48 PDT
Comment on attachment 108340 [details]
Fix bad merge

Rejecting attachment 108340 [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:
ore/bindings/scripts/test/JS/JSTestObj.cpp
patching file Source/WebCore/bindings/scripts/test/JS/JSTestObj.h
patching file Source/WebCore/bridge/qt/qt_instance.cpp
patching file Source/WebCore/bridge/qt/qt_runtime.cpp
patching file Source/WebCore/bridge/qt/qt_runtime.h
patching file Source/WebCore/workers/WorkerContext.h
patching file Source/autotools/symbols.filter

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/9769015
Comment 28 Mark Hahnenberg 2011-09-22 23:53:12 PDT
Created attachment 108445 [details]
Patch
Comment 29 Collabora GTK+ EWS bot 2011-09-23 01:30:08 PDT
Comment on attachment 108445 [details]
Patch

Attachment 108445 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9815120
Comment 30 Gustavo Noronha (kov) 2011-09-23 02:07:16 PDT
Comment on attachment 108445 [details]
Patch

Attachment 108445 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9811222
Comment 31 Mark Hahnenberg 2011-09-23 09:06:01 PDT
Created attachment 108482 [details]
Patch
Comment 32 WebKit Review Bot 2011-09-23 10:39:29 PDT
Comment on attachment 108482 [details]
Patch

Rejecting attachment 108482 [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:
o-playing-and-pause.html = MISSING PASS


Regressions: Unexpected image and text mismatch : (1)
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE



Full output: http://queues.webkit.org/results/9801615
Comment 33 Mark Hahnenberg 2011-09-23 10:46:05 PDT
(In reply to comment #32)
> (From update of attachment 108482 [details])
> Rejecting attachment 108482 [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:
> o-playing-and-pause.html = MISSING PASS
> 
> 
> Regressions: Unexpected image and text mismatch : (1)
>   svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT
> 
> Regressions: Unexpected image mismatch : (5)
>   fast/text/atsui-multiple-renderers.html = IMAGE
>   fast/text/international/danda-space.html = IMAGE
>   fast/text/international/thai-baht-space.html = IMAGE
>   fast/text/international/thai-line-breaks.html = IMAGE
>   platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE
> 
> 
> 
> Full output: http://queues.webkit.org/results/9801615

I don't think this is an actual failure :-(
Comment 34 Mark Hahnenberg 2011-09-23 12:39:31 PDT
Committed r95849: <http://trac.webkit.org/changeset/95849>