Summary: | Add static version of JSCell::visitChildren | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | ggaren, gustavo.noronha, gustavo, mrobinson, webkit.review.bot, xan.lopez | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||
Bug Blocks: | 67690, 68567 | ||||||||||||||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2011-09-19 15:39:11 PDT
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> |