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.
Created attachment 108072 [details] Patch
Created attachment 108074 [details] Resetting bindings
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.
(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.
> 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 on attachment 108074 [details] Resetting bindings r- for build failure and because I think the naming could be better.
Created attachment 108194 [details] Fixing windows
Created attachment 108199 [details] Patch
Comment on attachment 108199 [details] Patch Looks good. r=me if it builds and is not a perf. regression.
Created attachment 108211 [details] Benchmark results
Comment on attachment 108199 [details] Patch Attachment 108199 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9770894
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 on attachment 108199 [details] Patch Looks like gtk-ews has gone weird
(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.
Created attachment 108264 [details] Fix GTK
Comment on attachment 108264 [details] Fix GTK Attachment 108264 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9794063
Comment on attachment 108264 [details] Fix GTK Attachment 108264 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9793097
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 on attachment 108264 [details] Fix GTK r=me, assuming the GTK build failure really is a syntax error in the VERSION script.
(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.
Created attachment 108335 [details] Fix GTK for real
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 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 on attachment 108335 [details] Fix GTK for real Attachment 108335 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9798236
Created attachment 108340 [details] Fix bad merge
Comment on attachment 108340 [details] Fix bad merge r=me, assuming it builds.
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
Created attachment 108445 [details] Patch
Comment on attachment 108445 [details] Patch Attachment 108445 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9815120
Comment on attachment 108445 [details] Patch Attachment 108445 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9811222
Created attachment 108482 [details] Patch
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
(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 :-(
Committed r95849: <http://trac.webkit.org/changeset/95849>