Bug 68404 – Add static version of JSCell::visitChildren

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 108072 [details]
Patch

[Mark Hahnenberg]

Created attachment 108074 [details]
Resetting bindings

[Darin Adler]

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.

[Mark Hahnenberg]

(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.

[Geoffrey Garen]

> 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).

[Geoffrey Garen]

Comment on attachment 108074 [details]
Resetting bindings

r- for build failure and because I think the naming could be better.

[Mark Hahnenberg]

Created attachment 108194 [details]
Fixing windows

[Mark Hahnenberg]

Created attachment 108199 [details]
Patch

[Geoffrey Garen]

Comment on attachment 108199 [details]
Patch

Looks good. r=me if it builds and is not a perf. regression.

[Mark Hahnenberg]

Created attachment 108211 [details]
Benchmark results

[Gustavo Noronha (kov)]

Comment on attachment 108199 [details]
Patch

Attachment 108199 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9770894

[Mark Hahnenberg]

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?

[Oliver Hunt]

Comment on attachment 108199 [details]
Patch

Looks like gtk-ews has gone weird

[Martin Robinson]

(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.

[Mark Hahnenberg]

Created attachment 108264 [details]
Fix GTK

[Collabora GTK+ EWS bot]

Comment on attachment 108264 [details]
Fix GTK

Attachment 108264 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9794063

[Gustavo Noronha (kov)]

Comment on attachment 108264 [details]
Fix GTK

Attachment 108264 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9793097

[Geoffrey Garen]

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

[Geoffrey Garen]

Comment on attachment 108264 [details]
Fix GTK

r=me, assuming the GTK build failure really is a syntax error in the VERSION script.

[Martin Robinson]

(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.

[Mark Hahnenberg]

Created attachment 108335 [details]
Fix GTK for real

[Gustavo Noronha (kov)]

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

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[Mark Hahnenberg]

Created attachment 108340 [details]
Fix bad merge

[Geoffrey Garen]

Comment on attachment 108340 [details]
Fix bad merge

r=me, assuming it builds.

[WebKit Review Bot]

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

[Mark Hahnenberg]

Created attachment 108445 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 108445 [details]
Patch

Attachment 108445 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9815120

[Gustavo Noronha (kov)]

Comment on attachment 108445 [details]
Patch

Attachment 108445 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9811222

[Mark Hahnenberg]

Created attachment 108482 [details]
Patch

[WebKit Review Bot]

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

[Mark Hahnenberg]

(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 :-(

[Mark Hahnenberg]

Committed r95849: <http://trac.webkit.org/changeset/95849>