Created attachment 296945 [details] it might look like this

Geoff: OpaqueRootBarrier<> is how I expect the easy cases in the DOM to get handled, and I think that TrackBase is an example of an easy case.

I think that I might eventually make it so that there is a addOpaqueRoot() variant that takes OpaqueRoot<>. Not sure yet.

I'm just going to make addOpaqueRoot turn the object's visitChildren method into a constraint.

Created attachment 297026 [details] the patch

Comment on attachment 297026 [details] the patch r=me

Landed in https://trac.webkit.org/changeset/209766

Comment on attachment 297026 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=297026&action=review > Source/WebCore/bindings/js/JSXPathResultCustom.cpp:37 > void JSXPathResult::visitAdditionalChildren(JSC::SlotVisitor& visitor) > { > + visitor.rescanAsConstraint(); > + visitAdditionalChildren is an autogeneration entrypoint, invoked exclusively by CodeGeneratorJS.pm. I think it would be nice for CodeGeneratorJS.pm to autogenerate the call to rescanAsConstraint() before invoking visitAdditionalChildren. That way, life is even simpler for a DOM programmer -- just enumerate your stuff. In practice, every visitAdditionalChildren function needs rescanAsConstraint() -- and it's hard to imagine a function that visits arbitrary additional thingies in the DOM not needed rescanAsConstraint(). So I don't think we win anything by making this call optional.

(In reply to comment #8) > Comment on attachment 297026 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=297026&action=review > > > Source/WebCore/bindings/js/JSXPathResultCustom.cpp:37 > > void JSXPathResult::visitAdditionalChildren(JSC::SlotVisitor& visitor) > > { > > + visitor.rescanAsConstraint(); > > + > > visitAdditionalChildren is an autogeneration entrypoint, invoked exclusively > by CodeGeneratorJS.pm. > > I think it would be nice for CodeGeneratorJS.pm to autogenerate the call to > rescanAsConstraint() before invoking visitAdditionalChildren. That way, life > is even simpler for a DOM programmer -- just enumerate your stuff. > > In practice, every visitAdditionalChildren function needs > rescanAsConstraint() -- and it's hard to imagine a function that visits > arbitrary additional thingies in the DOM not needed rescanAsConstraint(). So > I don't think we win anything by making this call optional. That's one way to simplify it. The approach I'm converging to is to make JSC expose this API: - SlotVisitor::rescanAsConstraint makes the GC call JSCell::visitConstraints in the fixpoint. - The DOM puts most of its custom stuff inside visitConstraints, not visitChildren. In this world visitAdditionalChildren becomes visitAdditionalConstraints. I'll come back to this later. Right now I have bigger fish to fry.

There was a 50% Dromaeo JSLib regression in this range. It is not necessarily caused by this change but it is by far the most suspicious.

(In reply to comment #10) > There was a 50% Dromaeo JSLib regression in this range. It is not > necessarily caused by this change but it is by far the most suspicious. rdar://problem/29663107

Reverted r209766 for reason: Regressed Dromaeo JSLib by ~50% Committed r209812: <http://trac.webkit.org/changeset/209812>

(In reply to comment #12) > Reverted r209766 for reason: > > Regressed Dromaeo JSLib by ~50% > > Committed r209812: <http://trac.webkit.org/changeset/209812> This change fixes an ancient GC bug that makes our GC wrong in the case of generational or concurrent GC. We've had generational GC for a long time and we've had this bug for a long time. Basically, the DOM and GC were not coordinating tightly enough, causing the GC to presume that objects could be deleted in cases where they could not. These were DOM wrappers, so you don't crash - but if these DOM wrappers had any interesting state then it's observable. It's a bad semantics bug. So, I think we should reland this.

Comment on attachment 297026 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=297026&action=review > Source/JavaScriptCore/heap/Heap.cpp:521 > m_opaqueRoots.clear(); Note that, in the world of this patch, it is correct to clear m_opaqueRoots even in Eden collection, since (a) we know that the contents of m_opaqueRoots are not up-to-date anyway; and (b) we will rescan constraints in order to fill m_opaqueRoots correctly. Clearing this set even during Eden GC might help performance by reducing total heap size.

I'm going to see what happens if I reapply this patch now that we have a better story for constraint solver scheduling.

Comment on attachment 297026 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=297026&action=review >> Source/JavaScriptCore/heap/Heap.cpp:521 >> m_opaqueRoots.clear(); > > Note that, in the world of this patch, it is correct to clear m_opaqueRoots even in Eden collection, since (a) we know that the contents of m_opaqueRoots are not up-to-date anyway; and (b) we will rescan constraints in order to fill m_opaqueRoots correctly. > > Clearing this set even during Eden GC might help performance by reducing total heap size. Actually, not clearing them guarantees that we only have to reexecute the constraints once per GC in the common case. If we did clear them then we would have to reexecute the constraints twice per GC: once to populate the opaque root set and another time to confirm that it didn't change.

Created attachment 298529 [details] new version Still a work in progress.

It looks like in Dromaeo jslib, it takes ~1sec to process all of the constraints when they are represented in this very naive way.

It seems like we're only going at about 1 million constraints per second. That seems really slow. (For example, a sample jslib eden collection with 5429322 runs for 6186.234758 ms, and I've separately verified that almost all of that time is constraint scanning.)

I bet that any solution that allows me to scan these constraints in address order would be a big win. It's a big win for two reasons: 1) I think it's probably the only sensible way of doing a parallel scan. HashSet does not have the ability to do concurrent iteration. It's far better if the constraints are sharded somehow, and MarkedBlock is the perfect shard. 2) Processing them in address order means better locality. I bet we're losing a lot of perf due cache/TLB misses because of the random nature of a HashSet.

<rdar://problem/29993906>

Created attachment 298702 [details] last good version before BBoP refactoring

I just ran Dromaeo with a hacked version of the GC that ignores all addOpaqueRoot calls during the rescan-as-constraints constraint. This version is almost as slow as the version that honors those calls. This tells me that the dominant cost here is just the footwork of maintaining and traversing that darn HashSet of constraints. It really seems like one of the dominant costs here is the HashSet overhead. We can normally scan much more than 1 million objects per second.

Created attachment 298715 [details] beginning of BBoP refactoring

Created attachment 298738 [details] BBoP going smoothly so far

Created attachment 298751 [details] more things

Created attachment 298771 [details] it's starting to come together WebCore now creates its own JSDestructibleObjectSubspace for those wrappers that have output constraints, and then installs its own MarkingConstraint to process those output constraints.

Created attachment 298784 [details] sorta starting to compile

Created attachment 298785 [details] more!

Created attachment 298804 [details] jsc compiles Still making the rest of WK compile. Still haven't tested it.

Created attachment 298862 [details] it ran splay

Created attachment 298875 [details] string allocation is 7% faster

Holy cow, this revealed a terrible old bug: DOM global objects are never destructed.

(In reply to comment #33) > Holy cow, this revealed a terrible old bug: DOM global objects are never > destructed. Holy cow, I was completely wrong. JSGlobalObject installs a finalizer. This is super weird since JSSegmentedVariableObject is the one that should be installing the finalizer. Also this patch would obviate the need for such shenanigans. But, no real bug.

(In reply to comment #34) > (In reply to comment #33) > > Holy cow, this revealed a terrible old bug: DOM global objects are never > > destructed. > > Holy cow, I was completely wrong. > > JSGlobalObject installs a finalizer. > > This is super weird since JSSegmentedVariableObject is the one that should > be installing the finalizer. Also this patch would obviate the need for > such shenanigans. > > But, no real bug. Holy cow, I was wrong again! It IS a real bug because JSGlobalLexicalEnvironment. I think it's most correct to fix this bug by giving JSSegmentedVariableObject a m_classInfo pointer, creating a JSSegmentedVariableObjectSubspace, and using that subspace for all JSSegmentedVariableObject subclasses. But to do that, we need to make it so that it's only the destructor that cares about the fact that JSCell::classInfo() should sometimes be replaced with SomethingElse::classInfo(). In trunk, JSCell::classInfo() knows about these hacks so that it can be used from destructors. I'm trying to fix that...

Created attachment 298899 [details] fixed some bugs Also moved the actual allocator into MarkedAllocatorInlines.h, which is a new header. The GC is now moving in the direction of *Inlines.h files being just for the GC itself, which should be great for compile times when hacking on it.

This appears to dramatically reduce the Dromaeo regression. In DOM Modification Prototype, this appears to reduce GC pauses from several seconds to less than 100ms. And that's without any parallelization!

Prior to all of the subspace optimizations, this patch was a 2x regression on Dromaeo/jslib on my machine. With the subspace optimizations (so Subspaces for output constraints but no parallel scanning yet and wrappers are not lazy), this patch is a 1.12x regression on Dromaeo/jslib on my machine. I'll verify other benchmarks but this is looking pretty good already.

Created attachment 298917 [details] it's starting to look good Instead of a 2x jslib regression we have a 1.12x jslib regression.

Created attachment 298978 [details] testing I'm debugging some crashes and I want to see if EWS sees them.

Attachment 298978 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedBlockInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.h:83: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedAllocatorInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/SubspaceInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSStringSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/VisitingTimeout.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkingConstraint.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 9 in 73 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 298978 [details] testing Attachment 298978 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2900384 Number of test failures exceeded the failure limit.

Created attachment 298981 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 298978 [details] testing Attachment 298978 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2900413 Number of test failures exceeded the failure limit.

Created attachment 298982 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 298978 [details] testing Attachment 298978 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2900412 Number of test failures exceeded the failure limit.

Created attachment 298983 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 298978 [details] testing Attachment 298978 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2900411 Number of test failures exceeded the failure limit.

Created attachment 298985 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

I'm seeing a very strange crash in some tests that shows that we have heap corruption. Maybe two MarkedBlocks are cohabiting the same space. Maybe we dangle pointers to deleted MarkedBlocks. The possibilities are endless! Fortunately I have a test that repros this 100%.

Asan immediately found that the way I added LargeAllocation to Subspace was causing dangling pointers because I forgot how BasicRawSentinelNodes are supposed to destruct. That should fix some things.

Created attachment 298989 [details] more I think I fixed things. Let's see what happens.

Attachment 298989 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedBlockInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.h:83: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedAllocatorInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/SubspaceInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSStringSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/VisitingTimeout.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkingConstraint.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 9 in 73 files If any of these errors are false positives, please file a bug against check-webkit-style.

I think that I fixed all of the JSC test crashes - they were caused by the LargeAllocation snafu. There are still LayoutTests failures according to EWS.

Comment on attachment 298989 [details] more Attachment 298989 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2900872 Number of test failures exceeded the failure limit.

Created attachment 298993 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 298989 [details] more Attachment 298989 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2900876 Number of test failures exceeded the failure limit.

Created attachment 298994 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 298989 [details] more Attachment 298989 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2900886 Number of test failures exceeded the failure limit.

Created attachment 298995 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 298989 [details] more Attachment 298989 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2900890 Number of test failures exceeded the failure limit.

Created attachment 298996 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Looks like EWS was seeing an uninitialized field in Subspace! I think I fixed it.

Created attachment 299001 [details] possible patch

Attachment 299001 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedBlockInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.h:83: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedAllocatorInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/SubspaceInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSStringSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/VisitingTimeout.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkingConstraint.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 9 in 78 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 299004 [details] more fixes Fixed some build issues.

Attachment 299004 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedBlockInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.h:83: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedAllocatorInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/SubspaceInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSStringSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/VisitingTimeout.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkingConstraint.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 9 in 79 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 299035 [details] the patch

Attachment 299035 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedBlockInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.h:83: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedAllocatorInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/SubspaceInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSStringSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/VisitingTimeout.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkingConstraint.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 9 in 80 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 299037 [details] the patch

Attachment 299037 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedBlockInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.h:83: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedAllocatorInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/SubspaceInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSStringSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/VisitingTimeout.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkingConstraint.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 9 in 81 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 299037 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=299037&action=review > Source/JavaScriptCore/ChangeLog:68 > + Altogether, this means that we have some small progressions and only a small Dromaeo > + regression. The regression is due to the fact that we scan output constraints. Before the > + Subspace optimizations (see r209766, which was rolled out in r209812), this regression on > + Dromaeo/jslib was 2x but after that optimization it's only 1.12x. Would be good to put this in perspective by mentioning that we're talking about the version of the benchmark that leaks / abandons gigabytes of nodes. > Source/WebCore/bindings/js/WebCoreJSClientData.h:83 > +WEBCORE_EXPORT void initNormalWorldClientData(JSC::VM* vm); C:\cygwin\home\buildbot\WebKit\Source\WebCore\bindings\js\WebCoreJSClientData.h(83): error C2375: 'WebCore::initNormalWorldClientData': redefinition; different linkage (compiling source file C:\cygwin\home\buildbot\WebKit\Source\WebCore\DerivedSources.cpp) [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCoreDerivedSources.vcxproj] C:\cygwin\home\buildbot\WebKit\Source\WebCore\bindings\js\WebCoreJSClientData.h(36): note: see declaration of 'WebCore::initNormalWorldClientData' (compiling source file C:\cygwin\home\buildbot\WebKit\Source\WebCore\DerivedSources.cpp) > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1976 > + # which will execute all objects in the DOM's own outputConstraintSubspace. visit all objects? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1979 > + push(@headerContent, " static void visitOutputConstraints(JSCell*, JSC::SlotVisitor&);\n"); > + my $subspaceFunc = IsDOMGlobalObject($interface) ? "globalObjectOutputConstraintSubspaceFor" : "outputConstraintSubspaceFor"; > + push(@headerContent, " template<typename> static JSC::Subspace* subspaceFor(JSC::VM& vm) { return $subspaceFunc(vm); }\n"); It would be nice for the sake of DOM programmers to come up with some terminology that's more oriented to the client and less oriented to a mathematical description of the GC. Right now, we have "custom mark function", "visit children", "visit additional children", and "output constraint". Can we merge some of these?

(In reply to comment #72) > Comment on attachment 299037 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299037&action=review > > > Source/JavaScriptCore/ChangeLog:68 > > + Altogether, this means that we have some small progressions and only a small Dromaeo > > + regression. The regression is due to the fact that we scan output constraints. Before the > > + Subspace optimizations (see r209766, which was rolled out in r209812), this regression on > > + Dromaeo/jslib was 2x but after that optimization it's only 1.12x. > > Would be good to put this in perspective by mentioning that we're talking > about the version of the benchmark that leaks / abandons gigabytes of nodes. > > > Source/WebCore/bindings/js/WebCoreJSClientData.h:83 > > +WEBCORE_EXPORT void initNormalWorldClientData(JSC::VM* vm); > > C: > \cygwin\home\buildbot\WebKit\Source\WebCore\bindings\js\WebCoreJSClientData. > h(83): error C2375: 'WebCore::initNormalWorldClientData': redefinition; > different linkage (compiling source file > C:\cygwin\home\buildbot\WebKit\Source\WebCore\DerivedSources.cpp) > [C: > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCoreDerive > dSources.vcxproj] > > C: > \cygwin\home\buildbot\WebKit\Source\WebCore\bindings\js\WebCoreJSClientData. > h(36): note: see declaration of 'WebCore::initNormalWorldClientData' > (compiling source file > C:\cygwin\home\buildbot\WebKit\Source\WebCore\DerivedSources.cpp) > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1976 > > + # which will execute all objects in the DOM's own outputConstraintSubspace. > > visit all objects? > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1979 > > + push(@headerContent, " static void visitOutputConstraints(JSCell*, JSC::SlotVisitor&);\n"); > > + my $subspaceFunc = IsDOMGlobalObject($interface) ? "globalObjectOutputConstraintSubspaceFor" : "outputConstraintSubspaceFor"; > > + push(@headerContent, " template<typename> static JSC::Subspace* subspaceFor(JSC::VM& vm) { return $subspaceFunc(vm); }\n"); > > It would be nice for the sake of DOM programmers to come up with some > terminology that's more oriented to the client and less oriented to a > mathematical description of the GC. > > Right now, we have "custom mark function", "visit children", "visit > additional children", and "output constraint". Can we merge some of these? I should have said something in a comment. My idea was to have separate terminology for this in JSC and WebCore, with visitOutputConstraints being hidden from view most of the time. I think this patch does that except when CodeGeneratorJS overrides that method and initNormalWorld calls it in a constraint. Ideally, our object model would allow the DOM classes that have output constraints to subclass an "interface" declared in the DOM which declares a visitAdditionalChildren virtual method. Then the MarkingConstraint that the DOM adds would cast to that interface and call the visitAdditionalChildren method. In this ideal world, the explanation is not mathematical. It's something like: you put logic in visitAdditionalChildren if you want it to work with concurrent and generational GC. It's hard to put things in visitChildren since the CodeGenerator makes it awkward, so we can have some comment indicating the dangers of doing so but most people simply won't have to care. Unfortunately we don't have multiple inheritance or "interfaces" and we can't even declare virtual methods anywhere but all the way in JSCell. So, I called it visitOutputConstraints since that's good for JSC terminology, and the DOM just uses this to call visitAdditionalChildren. Incidentally this also means that visitAdditionalChildren can be a normal method (not one of our static virtual methods), doesn't need to call its Base version, and gets called from both visitChildren and visitOutputConstraint. If we wanted to avoid using math terms, we would need to come up with a DOMey way of saying: visit but not the first time. I like that the current solution incidentally means that we don't need to come up with a new term for this. It's just an output constraint. Therefore, I think that what's missing is just a comment that visitOutputConstraints is JSC-speak for revisiting the object if concurrency happened, and some rigor to use visitAdditionalChildren terminology in other WebCore comments. That way, the fact that visitOutputConstraints is a thing is not something you have to see unless you do care about the math. If we did want to use nonmathematical terminology then we risk making it confusing for non-WebCore clients of this functionality. Like, this is what CodeBlockSet is doing except better so we want to do something like this for CodeBlocks, and there it would be really useful to use the mathematical terms since CodeBlock has such a subtle bunch of mathematical constraints over Structhre transitions.

(In reply to comment #72) > Comment on attachment 299037 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299037&action=review > > > Source/JavaScriptCore/ChangeLog:68 > > + Altogether, this means that we have some small progressions and only a small Dromaeo > > + regression. The regression is due to the fact that we scan output constraints. Before the > > + Subspace optimizations (see r209766, which was rolled out in r209812), this regression on > > + Dromaeo/jslib was 2x but after that optimization it's only 1.12x. > > Would be good to put this in perspective by mentioning that we're talking > about the version of the benchmark that leaks / abandons gigabytes of nodes. Added. > > > Source/WebCore/bindings/js/WebCoreJSClientData.h:83 > > +WEBCORE_EXPORT void initNormalWorldClientData(JSC::VM* vm); > > C: > \cygwin\home\buildbot\WebKit\Source\WebCore\bindings\js\WebCoreJSClientData. > h(83): error C2375: 'WebCore::initNormalWorldClientData': redefinition; > different linkage (compiling source file > C:\cygwin\home\buildbot\WebKit\Source\WebCore\DerivedSources.cpp) > [C: > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCoreDerive > dSources.vcxproj] > > C: > \cygwin\home\buildbot\WebKit\Source\WebCore\bindings\js\WebCoreJSClientData. > h(36): note: see declaration of 'WebCore::initNormalWorldClientData' > (compiling source file > C:\cygwin\home\buildbot\WebKit\Source\WebCore\DerivedSources.cpp) I have a fix that moves initNormalWorldClient data into JSVMClientData, which obviates the need for a friend declaration, which obviates the need for doing WEBCORE_EXPORT in both places. > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1976 > > + # which will execute all objects in the DOM's own outputConstraintSubspace. > > visit all objects? Fixed! > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1979 > > + push(@headerContent, " static void visitOutputConstraints(JSCell*, JSC::SlotVisitor&);\n"); > > + my $subspaceFunc = IsDOMGlobalObject($interface) ? "globalObjectOutputConstraintSubspaceFor" : "outputConstraintSubspaceFor"; > > + push(@headerContent, " template<typename> static JSC::Subspace* subspaceFor(JSC::VM& vm) { return $subspaceFunc(vm); }\n"); > > It would be nice for the sake of DOM programmers to come up with some > terminology that's more oriented to the client and less oriented to a > mathematical description of the GC. > > Right now, we have "custom mark function", "visit children", "visit > additional children", and "output constraint". Can we merge some of these? Short answer: visitOutputConstraint is just the JSC name for visitAdditionalChildren, and we use visitOutputConstraint as the JSCell virtual method name since it's in JSC. I agree that we should merge "custom mark function" with some of the other jargon.

Created attachment 299096 [details] patch for landing This ought to fix the Windows build failure.

Created attachment 299101 [details] patch for landing Rebased

Attachment 299101 [details] did not pass style-queue: ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedBlockInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkedAllocatorInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/runtime/JSDestructibleObjectSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/SubspaceInlines.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/bindings/js/WebCoreJSClientData.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/runtime/JSStringSubspace.cpp:29: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebCore/ForwardingHeaders/heap/VisitingTimeout.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/WebCore/ForwardingHeaders/heap/MarkingConstraint.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 8 in 83 files If any of these errors are false positives, please file a bug against check-webkit-style.

Landed in http://trac.webkit.org/changeset/210844