Bug 165760

Summary: Make opaque root scanning truly constraint-based
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: WebCore JavaScriptAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, ap, barraclough, beidson, buildbot, cdumez, commit-queue, dbates, esprehn+autocc, ggaren, jsbell, kangil.han, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=165712
Bug Depends on: 167155    
Bug Blocks: 165909    
Attachments:
Description Flags
it might look like this
none
the patch
saam: review+
new version
none
last good version before BBoP refactoring
none
beginning of BBoP refactoring
none
BBoP going smoothly so far
none
more things
none
it's starting to come together
none
sorta starting to compile
none
more!
none
jsc compiles
none
it ran splay
none
string allocation is 7% faster
none
fixed some bugs
none
it's starting to look good
none
testing
buildbot: commit-queue-
Archive of layout-test-results from ews117 for mac-elcapitan
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
more
buildbot: commit-queue-
Archive of layout-test-results from ews101 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
possible patch
none
more fixes
none
the patch
none
the patch
ggaren: review+
patch for landing
none
patch for landing none

Description Filip Pizlo 2016-12-12 09:47:46 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2016-12-12 12:54:39 PST
Created attachment 296945 [details]
it might look like this
Comment 2 Filip Pizlo 2016-12-12 12:55:27 PST
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.
Comment 3 Filip Pizlo 2016-12-12 12:56:14 PST
I think that I might eventually make it so that there is a addOpaqueRoot() variant that takes OpaqueRoot<>.  Not sure yet.
Comment 4 Filip Pizlo 2016-12-12 14:49:11 PST
I'm just going to make addOpaqueRoot turn the object's visitChildren method into a constraint.
Comment 5 Filip Pizlo 2016-12-13 11:29:55 PST
Created attachment 297026 [details]
the patch
Comment 6 Saam Barati 2016-12-13 11:53:13 PST
Comment on attachment 297026 [details]
the patch

r=me
Comment 7 Filip Pizlo 2016-12-13 11:56:06 PST
Landed in https://trac.webkit.org/changeset/209766
Comment 8 Geoffrey Garen 2016-12-13 12:17:59 PST
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.
Comment 9 Filip Pizlo 2016-12-13 12:20:33 PST
(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.
Comment 10 Chris Dumez 2016-12-14 09:42:00 PST
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.
Comment 11 Chris Dumez 2016-12-14 09:45:43 PST
(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
Comment 12 Chris Dumez 2016-12-14 09:50:56 PST
Reverted r209766 for reason:

Regressed Dromaeo JSLib by ~50%

Committed r209812: <http://trac.webkit.org/changeset/209812>
Comment 13 Filip Pizlo 2016-12-14 09:56:30 PST
(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 14 Geoffrey Garen 2016-12-14 13:27:52 PST
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.
Comment 15 Filip Pizlo 2017-01-10 16:19:16 PST
I'm going to see what happens if I reapply this patch now that we have a better story for constraint solver scheduling.
Comment 16 Filip Pizlo 2017-01-10 16:52:01 PST
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.
Comment 17 Filip Pizlo 2017-01-10 17:22:05 PST
Created attachment 298529 [details]
new version

Still a work in progress.
Comment 18 Filip Pizlo 2017-01-12 09:28:47 PST
It looks like in Dromaeo jslib, it takes ~1sec to process all of the constraints when they are represented in this very naive way.
Comment 19 Filip Pizlo 2017-01-12 10:09:52 PST
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.)
Comment 20 Filip Pizlo 2017-01-12 10:20:33 PST
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.
Comment 21 Radar WebKit Bug Importer 2017-01-12 10:44:10 PST
<rdar://problem/29993906>
Comment 22 Filip Pizlo 2017-01-12 11:48:05 PST
Created attachment 298702 [details]
last good version before BBoP refactoring
Comment 23 Filip Pizlo 2017-01-12 13:32:49 PST
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.
Comment 24 Filip Pizlo 2017-01-12 14:57:12 PST
Created attachment 298715 [details]
beginning of BBoP refactoring
Comment 25 Filip Pizlo 2017-01-12 17:00:02 PST
Created attachment 298738 [details]
BBoP going smoothly so far
Comment 26 Filip Pizlo 2017-01-12 21:17:47 PST
Created attachment 298751 [details]
more things
Comment 27 Filip Pizlo 2017-01-13 11:05:28 PST
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.
Comment 28 Filip Pizlo 2017-01-13 13:01:46 PST
Created attachment 298784 [details]
sorta starting to compile
Comment 29 Filip Pizlo 2017-01-13 13:54:53 PST
Created attachment 298785 [details]
more!
Comment 30 Filip Pizlo 2017-01-13 15:59:35 PST
Created attachment 298804 [details]
jsc compiles

Still making the rest of WK compile.  Still haven't tested it.
Comment 31 Filip Pizlo 2017-01-14 15:24:53 PST
Created attachment 298862 [details]
it ran splay
Comment 32 Filip Pizlo 2017-01-14 19:34:39 PST
Created attachment 298875 [details]
string allocation is 7% faster
Comment 33 Filip Pizlo 2017-01-14 22:07:18 PST
Holy cow, this revealed a terrible old bug: DOM global objects are never destructed.
Comment 34 Filip Pizlo 2017-01-14 22:35:41 PST
(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.
Comment 35 Filip Pizlo 2017-01-15 09:34:10 PST
(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...
Comment 36 Filip Pizlo 2017-01-15 10:23:05 PST
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.
Comment 37 Filip Pizlo 2017-01-15 12:00:25 PST
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!
Comment 38 Filip Pizlo 2017-01-15 12:10:37 PST
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.
Comment 39 Filip Pizlo 2017-01-15 12:12:02 PST
Created attachment 298917 [details]
it's starting to look good

Instead of a 2x jslib regression we have a 1.12x jslib regression.
Comment 40 Filip Pizlo 2017-01-16 11:55:37 PST
Created attachment 298978 [details]
testing

I'm debugging some crashes and I want to see if EWS sees them.
Comment 41 WebKit Commit Bot 2017-01-16 11:57:55 PST
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 42 Build Bot 2017-01-16 12:58:14 PST
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.
Comment 43 Build Bot 2017-01-16 12:58:20 PST
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 44 Build Bot 2017-01-16 13:02:41 PST
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.
Comment 45 Build Bot 2017-01-16 13:02:53 PST
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 46 Build Bot 2017-01-16 13:04:02 PST
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.
Comment 47 Build Bot 2017-01-16 13:04:08 PST
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 48 Build Bot 2017-01-16 13:10:48 PST
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.
Comment 49 Build Bot 2017-01-16 13:10:54 PST
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
Comment 50 Filip Pizlo 2017-01-16 13:21:49 PST
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%.
Comment 51 Filip Pizlo 2017-01-16 13:30:40 PST
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.
Comment 52 Filip Pizlo 2017-01-16 14:05:44 PST
Created attachment 298989 [details]
more

I think I fixed things.  Let's see what happens.
Comment 53 WebKit Commit Bot 2017-01-16 14:12:43 PST
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.
Comment 54 Filip Pizlo 2017-01-16 14:44:00 PST
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 55 Build Bot 2017-01-16 15:14:55 PST
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.
Comment 56 Build Bot 2017-01-16 15:15:03 PST
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 57 Build Bot 2017-01-16 15:17:00 PST
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.
Comment 58 Build Bot 2017-01-16 15:17:09 PST
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 59 Build Bot 2017-01-16 15:21:31 PST
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.
Comment 60 Build Bot 2017-01-16 15:21:39 PST
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 61 Build Bot 2017-01-16 15:26:47 PST
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.
Comment 62 Build Bot 2017-01-16 15:26:55 PST
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
Comment 63 Filip Pizlo 2017-01-16 16:25:52 PST
Looks like EWS was seeing an uninitialized field in Subspace!  I think I fixed it.
Comment 64 Filip Pizlo 2017-01-16 16:46:52 PST
Created attachment 299001 [details]
possible patch
Comment 65 WebKit Commit Bot 2017-01-16 16:55:38 PST
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.
Comment 66 Filip Pizlo 2017-01-16 17:26:21 PST
Created attachment 299004 [details]
more fixes

Fixed some build issues.
Comment 67 WebKit Commit Bot 2017-01-16 17:28:58 PST
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.
Comment 68 Filip Pizlo 2017-01-17 08:54:01 PST
Created attachment 299035 [details]
the patch
Comment 69 WebKit Commit Bot 2017-01-17 08:56:41 PST
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.
Comment 70 Filip Pizlo 2017-01-17 09:19:18 PST
Created attachment 299037 [details]
the patch
Comment 71 WebKit Commit Bot 2017-01-17 09:22:07 PST
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 72 Geoffrey Garen 2017-01-17 16:03:37 PST
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?
Comment 73 Filip Pizlo 2017-01-17 16:24:56 PST
(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.
Comment 74 Filip Pizlo 2017-01-17 18:02:35 PST
(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.
Comment 75 Filip Pizlo 2017-01-17 18:06:26 PST
Created attachment 299096 [details]
patch for landing

This ought to fix the Windows build failure.
Comment 76 Filip Pizlo 2017-01-17 18:16:52 PST
Created attachment 299101 [details]
patch for landing

Rebased
Comment 77 WebKit Commit Bot 2017-01-17 18:20:56 PST
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.
Comment 78 Filip Pizlo 2017-01-17 20:25:29 PST
Landed in http://trac.webkit.org/changeset/210844