We're at the point where we can create an option to run the GC concurrently and make it default to false.
This bug will start with the patch of https://bugs.webkit.org/show_bug.cgi?id=163734; I may just dup that to this and land a combined patch.
There are two things that definitely need to get done for this to be correct:
1) Separate the SlotVisitor used by the mutator's write barrier from the SlotVisitor used by the collector thread. It's not super clear if the write barrier should even use a SlotVisitor, but whatever - the MarkStackArray probably has everything we need to efficiently transfer the entire contents of the mutator SlotVisitor into the collector SlotVisitor.
2) We need to stopAllocating/resumeAllocating. It's tempting to think that this should be done by the mutator, but that's actually just complicating things. It should be the collector's responsibility, and I think that these methods should be called from stopTheWorld/resumeTheWorld.
Created attachment 294004[details]
it actually sort of did some things
It started a concurrent eden collection, then the fixpoint went off the rails and we bloated memory to 500MB. Then it did another GC and crashed.
That's a pretty good start.
Created attachment 294007[details]
it sort of works
This patch is pretty broken, but it's doing concurrent collections in Octane/splay. It crashes eventually of course.
I'm making some progress, but I just realized that the whole OldGrey/NewGrey state arrangement is busted. We need some other way of knowing if we're scanning an object due to the barrier versus due to marking. I think that the only way to do it is to have separate mark stacks for the two things.
The GC is seeing objects with totally corrupt structure IDs.
Before going into this further, I'm going to add a collectContinuously mode. In this mode the GC will just always be running. After finishing a cycle it will immediately start another one. This way, the GC will be able to more effectively tell me what is going wrong.
I'm debugging with generational GC turned off, for now. I'm also running in baseline JIT only, just to keep my reasoning about barriers super simple.
(In reply to comment #12)
> Oh wow, MarkedBlock::stopAllocating() is totally wrong: it blows away
> newlyAllocated!
Hmmm, actually, no. It blows it away but then it makes it right.
For a minute I was thinking that this was something that could only be right if you ran it at the top of GC and now just after the other increments. But now I'm thinking it should work no matter what. I'll have to stare at it more.
Created attachment 294074[details]
starting to work with useConcurrentJIT=false
Since it can collect concurrently like seven times, I'm going to focus on getting it to work properly with concurrent JIT disabled. That's shaking out some great bugs.
(In reply to comment #15)
> Created attachment 294074[details]
> starting to work with useConcurrentJIT=false
>
> Since it can collect concurrently like seven times, I'm going to focus on
> getting it to work properly with concurrent JIT disabled. That's shaking out
> some great bugs.
This patch has concurrent GC turned off by default. But even with it turned off, the collector uses the concurrent GC algorithm - it just happens to not stop the world. As, the resumeTheWorld/stopTheWorld steps that let the mutator "peek" into the collector become no-ops by default.
Running in the default mode, this patch passes release stress tests. I think it would pass debug stress tests, but I didn't have time to run that yet. Next I'm going to run perf tests. I want to see if the algorithm changes have any downside. I moved *everything* into the fixpoint, which sounds completely insane but will probably work.
(In reply to comment #18)
> I need to make all of CodeBlock's visitChildren logic thread-safe. It's not
> right now, but easily could be - there's already a lock for that.
Alternatively, I could have a revisitWithWorldStopped hook in SlotVisitor, and we could make the "bad" visitChidren methods use this. You would use it like this:
void Blah::visitChildren(SlotVisitor& visitor)
{
if (visitor.revisitWithWorldStopped())
return;
... // some terrible race conditions
}
Where revisitWithWorldStopped() would initially check if the world is stopped already, and if it is, it would schedule the object to be revisited on a separate visit-while-world-is-stopped list. When visited from that worklist, it would return false.
I'm not sure yet if I will need this.
For objects that change structure every time a new property is added, the current regime of loading the structure and butterfly in the right order in the collector, and storing them in the right order in the mutator, totally works.
But this breaks as soon as we add properties without transition. This happens for dictionaries and as an optimization.
It seems that the Structure is already grabbing locks in this case. Maybe the right thing to do is to do the entire store and butterfly switcheroo while that lock is held. Then have the GC scan hold the Structure's lock if it's a dictionary or something.
Created attachment 294098[details]
fixing races in JSObject::visitChildren
If the main thread is allocating a butterfly while the GC is scanning the object then we have a funny concurrent copying-like case. Fortunately, it's easy to fix.
If we're not in dictionary mode, then we just want the GC to see the old structure until the end of the transition. That's easy.
If we're in dictionary mode, then any dangerous mutator activity will already grab the Structure's lock. So, we just need to slave the other stuff inside the lock. So the lock will protect the object.
(In reply to comment #21)
> Created attachment 294098[details]
> fixing races in JSObject::visitChildren
>
> If the main thread is allocating a butterfly while the GC is scanning the
> object then we have a funny concurrent copying-like case. Fortunately, it's
> easy to fix.
>
> If we're not in dictionary mode, then we just want the GC to see the old
> structure until the end of the transition. That's easy.
>
> If we're in dictionary mode, then any dangerous mutator activity will
> already grab the Structure's lock. So, we just need to slave the other stuff
> inside the lock. So the lock will protect the object.
Aw snap, but what about arrays?
If we have indexed storage then we need to have some way of scanning the right subset of it.
One option is to resize arrays under a lock. Maybe that would work.
Another option is to institute proper discipline with respect to the lengths: don't increase length until we've sanitized that space.
(In reply to comment #22)
> (In reply to comment #21)
> > Created attachment 294098[details]
> > fixing races in JSObject::visitChildren
> >
> > If the main thread is allocating a butterfly while the GC is scanning the
> > object then we have a funny concurrent copying-like case. Fortunately, it's
> > easy to fix.
> >
> > If we're not in dictionary mode, then we just want the GC to see the old
> > structure until the end of the transition. That's easy.
> >
> > If we're in dictionary mode, then any dangerous mutator activity will
> > already grab the Structure's lock. So, we just need to slave the other stuff
> > inside the lock. So the lock will protect the object.
>
> Aw snap, but what about arrays?
>
> If we have indexed storage then we need to have some way of scanning the
> right subset of it.
>
> One option is to resize arrays under a lock. Maybe that would work.
>
> Another option is to institute proper discipline with respect to the
> lengths: don't increase length until we've sanitized that space.
Let me just elaborate. With indexing storage, two kinds of things can happen that would make us nervous:
1) The butterfly is reallocated. But this isn't actually scary because the array butterfly describes its own indexing storage length.
2) The lengths change. Here we just need to ensure that when increasing the length, we clear the wilderness before actually changing the length. It may be that we do this already, I will audit.
(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > Created attachment 294098[details]
> > > fixing races in JSObject::visitChildren
> > >
> > > If the main thread is allocating a butterfly while the GC is scanning the
> > > object then we have a funny concurrent copying-like case. Fortunately, it's
> > > easy to fix.
> > >
> > > If we're not in dictionary mode, then we just want the GC to see the old
> > > structure until the end of the transition. That's easy.
> > >
> > > If we're in dictionary mode, then any dangerous mutator activity will
> > > already grab the Structure's lock. So, we just need to slave the other stuff
> > > inside the lock. So the lock will protect the object.
> >
> > Aw snap, but what about arrays?
> >
> > If we have indexed storage then we need to have some way of scanning the
> > right subset of it.
> >
> > One option is to resize arrays under a lock. Maybe that would work.
> >
> > Another option is to institute proper discipline with respect to the
> > lengths: don't increase length until we've sanitized that space.
>
> Let me just elaborate. With indexing storage, two kinds of things can
> happen that would make us nervous:
>
> 1) The butterfly is reallocated. But this isn't actually scary because the
> array butterfly describes its own indexing storage length.
>
> 2) The lengths change. Here we just need to ensure that when increasing the
> length, we clear the wilderness before actually changing the length. It may
> be that we do this already, I will audit.
Yeah, this turned out to be easy, I think.
It seems that we'll need a lock for ArrayStorage unshifting. This lock can't go into the ArrayStorage itself since that's what's being shifted, so we need to put the lock into the JSCell header. We've seen this coming, so this shouldn't be too bad.
Then there are these two transitions, that seem like they need to happen under a lock:
Contiguous -> ArrayStorage
Contiguous -> SlowPutArrayStorage
But maybe not? What if the mutator protocol here was:
1) Build the ArrayStorage butterfly but artificially set the lengths to zero.
2) Install the ArrayStorage butterfly.
3) Change the structure.
4) Set the lengths to something real.
If the GC interleaves between (2) and (3), it will think that it has Contiguous with zero length.
If the GC interleaves between (3) and (4), it will think that it has ArrayStorage with zero length.
These are all fine outcomes because the act of changing the structure will trigger a barrier that causes this object to be rescanned.
I'm now to the point where I can run ~20 GCs in debug mode with collectContinuously=true.
The next problem is that Structure property tables are getting cleared by GC! This turns out to be easier to fix than it seems.
(In reply to comment #33)
> I'm now to the point where I can run ~20 GCs in debug mode with
> collectContinuously=true.
>
> The next problem is that Structure property tables are getting cleared by
> GC! This turns out to be easier to fix than it seems.
I started working on a fix that involved more locking, but I think that literally all we want to do is this:
- materializePropertyMap returns the map it created.
- Nobody accesses propertyTable() directly. We might as well kill the method.
(In reply to comment #35)
> Created attachment 294324[details]
> more
>
> Hopefully fixes races in Structure. I haven't tested it yet so it's
> probably totally broken.
It now crashed after 40 GCs. Progress!
(In reply to comment #42)
> Created attachment 294449[details]
> more
This looks like it's passing all tests with concurrent GC disabled.
That's pretty impressive considering that I basically redid how butterfly resizing works, how structure transition work, and how property tables work. And a bunch of other random stuff.
I'll need to perf test the heck out of it.
(In reply to comment #45)
> I fixed the perf regression - it was a goof-up in
> Structure::willStoreValueSlow.
Nope, there are still regressions in raytrace and earley-boyer. Investigating...
(In reply to comment #46)
> (In reply to comment #45)
> > I fixed the perf regression - it was a goof-up in
> > Structure::willStoreValueSlow.
>
> Nope, there are still regressions in raytrace and earley-boyer.
> Investigating...
Fixed raytrace. I need PutStructure to have a store-store fence, which means "read world" in B3, so that's what I did in DFG. But in DFG, reading "world" also means reading the Stack. Saying read(Heap) fixes the regression by reenabling PutStack sinking.
In reality, we can do a lot better than read(Heap). I think we just need read(JSCell_butterfly). We're just trying to ensure that the collector sees the new butterfly before it sees the new structure.
(In reply to comment #47)
> (In reply to comment #46)
> > (In reply to comment #45)
> > > I fixed the perf regression - it was a goof-up in
> > > Structure::willStoreValueSlow.
> >
> > Nope, there are still regressions in raytrace and earley-boyer.
> > Investigating...
>
> Fixed raytrace. I need PutStructure to have a store-store fence, which
> means "read world" in B3, so that's what I did in DFG. But in DFG, reading
> "world" also means reading the Stack. Saying read(Heap) fixes the
> regression by reenabling PutStack sinking.
>
> In reality, we can do a lot better than read(Heap). I think we just need
> read(JSCell_butterfly). We're just trying to ensure that the collector sees
> the new butterfly before it sees the new structure.
And in B3, we want the fence to read both cell and butterfly, but if we do that, we might as well tell B3 that we read top.
Attachment 294512[details] did not pass style-queue:
ERROR: Source/WTF/wtf/LockAlgorithm.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:178: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:508: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:549: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:552: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:555: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:560: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:576: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:580: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:761: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:676: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:676: The parameter name "table" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 12 in 63 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 294526[details] did not pass style-queue:
ERROR: Source/WTF/wtf/LockAlgorithm.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:178: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:508: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:549: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:552: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:555: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:560: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:576: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:580: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:761: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:676: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:676: The parameter name "table" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 12 in 64 files
If any of these errors are false positives, please file a bug against check-webkit-style.
r=me
> Source/JavaScriptCore/dfg/DFGTierUpCheckInjectionPhase.cpp:67
> +
Revert.
> Source/JavaScriptCore/heap/CellState.h:40
> + // The object is going to be is in eden. During GC, this means that the object has not been marked yet.
I think you meant "is going to be in eden".
I'm not sure what you mean by the future tense here. Where are you before you are in eden?
> Source/JavaScriptCore/heap/ResumeTheWorldScope.h:24
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> + * THE POSSIBILITY OF SUCH DAMAGE.
> + */
Where's the rest of this? :P
> Source/JavaScriptCore/jit/Repatch.cpp:221
> +
Revert.
> Source/JavaScriptCore/runtime/JSCell.h:121
> + void lockInternalLock();
> + void unlockInternalLock();
> +
> + class InternalLocker {
> + public:
> + InternalLocker(JSCell* cell)
> + : m_cell(cell)
> + {
> + m_cell->lockInternalLock();
> + }
> +
> + ~InternalLocker()
> + {
> + m_cell->unlockInternalLock();
> + }
Why the special naming instead of WTF::Locker and lock()/unlock()?
I'm not sure that it's meaningful to call out this lock as "internal". We're making it public and claiming that it's available if you need it.
> Source/JavaScriptCore/runtime/JSCell.h:220
> + // DO NOT store to this field. Always use a CAS loop, since some bits are flipped using CAS
Why are you yelling at me?
> Source/JavaScriptCore/runtime/JSObject.cpp:782
> + WTF::storeStoreFence();
> + setButterfly(vm, newButterfly);
> + setStructure(vm, newStructure);
A little bit surprising that setStructure builds in a fence at the top but setButterfly doesn't. Kind of feels like either both should or neither should.
When I first read this code, it looked like an obvious missing fence between setButterfly and setStructure.
> Source/JavaScriptCore/runtime/Structure.h:192
> + // These should be used with caution. Versions that take a func will call it after making the
I would remove "These should be used with caution". Abstract anxiety is no way to program.
> Source/JavaScriptCore/runtime/Structure.h:673
> + // This will grab the lock. DO NOT call when holding the Structure's lock.
Why yelling?
(In reply to comment #54)
> r=me
>
> > Source/JavaScriptCore/dfg/DFGTierUpCheckInjectionPhase.cpp:67
> > +
>
> Revert.
>
> > Source/JavaScriptCore/heap/CellState.h:40
> > + // The object is going to be is in eden. During GC, this means that the object has not been marked yet.
>
> I think you meant "is going to be in eden".
>
> I'm not sure what you mean by the future tense here. Where are you before
> you are in eden?
Fixed.
>
> > Source/JavaScriptCore/heap/ResumeTheWorldScope.h:24
> > + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> > + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
> > + * THE POSSIBILITY OF SUCH DAMAGE.
> > + */
>
> Where's the rest of this? :P
I moved it to Heap.h/cpp as a nested class of Heap. I'll remove this file.
>
> > Source/JavaScriptCore/jit/Repatch.cpp:221
> > +
>
> Revert.
Reverted.
>
> > Source/JavaScriptCore/runtime/JSCell.h:121
> > + void lockInternalLock();
> > + void unlockInternalLock();
> > +
> > + class InternalLocker {
> > + public:
> > + InternalLocker(JSCell* cell)
> > + : m_cell(cell)
> > + {
> > + m_cell->lockInternalLock();
> > + }
> > +
> > + ~InternalLocker()
> > + {
> > + m_cell->unlockInternalLock();
> > + }
>
> Why the special naming instead of WTF::Locker and lock()/unlock()?
I had it this way at first and it was super annoying:
- It would mean also implementing isHeld() or isLocked() or any other extra methods that Locker wants. This approach frees us up to add more crap to Locker/Lock without having to worry about this lock also implementing all of those things.
- Writing JSCell::Locker locker(*cell), or Locker<JSCell> locker(*cell), led to awkward looking code. The trouble is that many cells, like Structure, also have an explicit lock for their state. I think that we want to encourage most locking scenarios in subclasses of JSCell to introduce their own lock unless saving that one byte is actually meaningful. That's because the more people use this cell lock, the more of a chance we have of overlocking leading to deadlocks.
- Writing cell->lock()/cell->unlock() in algorithms that cannot do structured locking was too much for me. Considering all of the powers that cells have, "locking" them could mean so many things here. When I first read it back to myself it felt like the "locking" had some other meaning, like that it was another word for "pinning". That's how it reads when you see it in visitChildren().
I wanted unstructured locking code to look like it's actually locking a lock. Then the other benefits - not having to implement all of the things that Locker/Lock do for each other and a more explicit syntax for structured locking - were icing.
>
> I'm not sure that it's meaningful to call out this lock as "internal". We're
> making it public and claiming that it's available if you need it.
Not quite. A public lock would be like the Java object monitor, which is a user-visible (as in, Java has syntax for locking it) lock. This lock is not visible in that way and never will be. It's for locking internal stuff that the user does not see.
It's important to call out the fact that this lock is already used internally, so that other clients know that they have to consider if their use of it would cause overlocking.
>
> > Source/JavaScriptCore/runtime/JSCell.h:220
> > + // DO NOT store to this field. Always use a CAS loop, since some bits are flipped using CAS
>
> Why are you yelling at me?
I'll downcase it.
>
> > Source/JavaScriptCore/runtime/JSObject.cpp:782
> > + WTF::storeStoreFence();
> > + setButterfly(vm, newButterfly);
> > + setStructure(vm, newStructure);
>
> A little bit surprising that setStructure builds in a fence at the top but
> setButterfly doesn't. Kind of feels like either both should or neither
> should.
A fraction of setButterfly() clients either avoid the fence altogether or accomplish the same effect through other means.
>
> When I first read this code, it looked like an obvious missing fence between
> setButterfly and setStructure.
Yeah, it's weird. I want to get a bit more experience with this before I come up with some idiom.
>
> > Source/JavaScriptCore/runtime/Structure.h:192
> > + // These should be used with caution. Versions that take a func will call it after making the
>
> I would remove "These should be used with caution". Abstract anxiety is no
> way to program.
Removed.
>
> > Source/JavaScriptCore/runtime/Structure.h:673
> > + // This will grab the lock. DO NOT call when holding the Structure's lock.
>
> Why yelling?
Fixed.
Attachment 294616[details] did not pass style-queue:
ERROR: Source/WTF/wtf/LockAlgorithm.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:178: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:509: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:549: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:552: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:555: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:560: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:576: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:580: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:761: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "table" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 12 in 66 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 294623[details] did not pass style-queue:
ERROR: Source/WTF/wtf/LockAlgorithm.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:178: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:509: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:549: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:552: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:555: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:560: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:576: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:580: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:761: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "table" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 12 in 65 files
If any of these errors are false positives, please file a bug against check-webkit-style.
The store barriers on PutStructure look like they're too much for ARM. GC performance is also hurt but not nearly as much as construction time.
Something I can do is teach the DFG and B3 more about the fence in PutStructure. For example, we don't need it at all if we hadn't messed with the butterfly before. Also, currently the fence removes our ability to do store elimination on PutStructures, which means that we'll have a long chain of totally unnecessary fences in any object constructor.
(In reply to comment #63)
> The store barriers on PutStructure look like they're too much for ARM. GC
> performance is also hurt but not nearly as much as construction time.
>
> Something I can do is teach the DFG and B3 more about the fence in
> PutStructure. For example, we don't need it at all if we hadn't messed with
> the butterfly before. Also, currently the fence removes our ability to do
> store elimination on PutStructures, which means that we'll have a long chain
> of totally unnecessary fences in any object constructor.
If we always initialize objects upon allocation so that the wilderness of not-yet-initialized properties is zero-filled then we don't need fences when setting the structure. That's pretty sweet.
Last time I plated with this, adding code to initialize stuff upon allocation was not expensive.
I'm attempting this now.
VICTORY! I'm testing the revised version of the patch that attempts to address ARM's woes. For the first time, I worked up the courage to test the concurrent GC in anger.
This is the performance of splay normally (tip of tree or with my patch and --useConcurrentGC=false [i.e. the default]):
[pizlo@murderface OpenSource] DYLD_FRAMEWORK_PATH=WebKitBuild/Release/ WebKitBuild/Release/jsc splay.js
That took 3298.9039421081543 ms.
Above 99.9%: 11.776494979858398 ms.
Above 99%: 10.036780834197998 ms.
Above 97.5%: 4.4260969161987305 ms.
Above 95%: 2.412609100341797 ms.
Above 90%: 1.3552560806274414 ms.
Above 75%: 0.688254451751709 ms.
Above 50%: 0.4552687644958496 ms.
Above 0%: 0.3296610832214355 ms.
"Above 0%" is a funny way of saying "average". Notice that normally, there is a 1% chance that one tick of splay will take about 10ms, while usually a tick of splay will take 0.33ms. That's a ~30x slow-down that you might randomly experience while doing something in JavaScript, all because the garbage collector interrupted you.
This is the performance of splay with concurrent GC:
[pizlo@murderface OpenSource] DYLD_FRAMEWORK_PATH=WebKitBuild/Release/ WebKitBuild/Release/jsc splay.js --useConcurrentGC=true
That took 3011.0859870910645 ms.
Above 99.9%: 5.75716495513916 ms.
Above 99%: 2.2775816917419434 ms.
Above 97.5%: 1.5731096267700195 ms.
Above 95%: 1.1017203330993652 ms.
Above 90%: 0.7723274230957031 ms.
Above 75%: 0.5373270988464356 ms.
Above 50%: 0.39242477416992183 ms.
Above 0%: 0.3008724212646484 ms.
First of all, notice the "That took" number and ponder its wonderful significance. This is incredible: splay with concurrent GC runs to completion faster than splay without concurrent GC. I did not know that this was possible. All of my previous investigations into concurrent GC showed that having concurrent collector activity slows you down, but about 5%, due to the combination of increased barrier costs, increased allocation costs, memory thrashing (the GC touches all of the heap at least once and this creates tons of contention with the mutator), and extra interruptions (the concurrent GC will tell the mutator to do things more times total than a non-concurrent one). This GC was indeed engineered to minimize all of those things, except for barrier costs, which are much higher when the collector is running (2x worse if you isolate the cost of a heap cell store), and interruptions (it's just as bad as grey-stack CMR [my last concurrent GC] - they both interrupt the mutator about 5-10 times per cycle). It turns out that choosing the crazy Steele-barrier GC strategy worked decisively: splay seems to run between 5-10% *faster* with concurrent GC enabled! I think this is mostly because I avoided imposing any allocation tax, because the GC is parallel and is engineered to reduce the number of times it touches memory, and because the Steele barrier obliterates the floating garbage problem.
It's clear that the concurrency is doing its job. It's also clear that there is room for improvement. In the 1% case, your time to completion drops from 10ms to 2.28ms. That's a pretty good improvement, but I was hoping for more. Basically every probability bucket improves, all the way down to the average.
I think that the way to reduce worst-case times further is to reduce barrier costs and add a bit of time slicing to the collector's self-imposed pauses.
Attachment 294724[details] did not pass style-queue:
ERROR: Source/WTF/wtf/LockAlgorithm.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:178: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "table" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:509: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:549: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:552: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:555: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:560: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:576: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:580: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:761: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 12 in 71 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 294733[details]
Archive of layout-test-results from ews114 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #71)
> Created attachment 294733[details]
> Archive of layout-test-results from ews114 for mac-yosemite
>
> The attached test failures were seen while running run-webkit-tests on the
> mac-debug-ews.
> Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
I think I have a fix for this.
Attachment 294738[details] did not pass style-queue:
ERROR: Source/WTF/wtf/LockAlgorithm.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:178: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "table" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:509: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:554: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:557: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:560: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:565: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:581: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:585: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:761: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 12 in 71 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 294739[details] did not pass style-queue:
ERROR: Source/WTF/wtf/LockAlgorithm.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:178: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "table" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:509: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:554: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:557: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:560: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:565: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:581: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:585: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:761: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 12 in 69 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 294740[details] did not pass style-queue:
ERROR: Source/WTF/wtf/LockAlgorithm.h:26: Use #pragma once instead of #ifndef for header guard. [build/header_guard] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:178: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "vm" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/Structure.h:675: The parameter name "table" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:509: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:554: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:557: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:560: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:565: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:581: Multi line control clauses should use braces. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/Heap.cpp:585: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:761: The parameter name "structure" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 12 in 70 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 294740[details]
more fixes
View in context: https://bugs.webkit.org/attachment.cgi?id=294740&action=review
r=me
I don't fully understand the fencing rules here. Let's discuss them in a group meeting. (We'll need everybody to understand these rules in the end.)
> PerformanceTests/ES6SampleBench/Air/benchmark.js:81
> + print("iteration " + iteration);
Revert.
There was a 1% Octane regression on Mac in the range where this landed. I have not identified which commit caused it but this patch seems the most suspicious in the range.
(In reply to comment #84)
> There was a 1% Octane regression on Mac in the range where this landed. I
> have not identified which commit caused it but this patch seems the most
> suspicious in the range.
EarleyBoyer: 5% worse
pdfJS: 6% worse
RayTrace: 6% worse
(In reply to comment #85)
> (In reply to comment #84)
> > There was a 1% Octane regression on Mac in the range where this landed. I
> > have not identified which commit caused it but this patch seems the most
> > suspicious in the range.
>
> EarleyBoyer: 5% worse
> pdfJS: 6% worse
> RayTrace: 6% worse
Filed <rdar://problem/29281375>.
2016-11-05 12:05 PDT, Filip Pizlo
2016-11-05 14:05 PDT, Filip Pizlo
2016-11-05 15:33 PDT, Filip Pizlo
2016-11-06 18:36 PST, Filip Pizlo
2016-11-06 19:17 PST, Filip Pizlo
2016-11-07 11:42 PST, Filip Pizlo
2016-11-07 12:11 PST, Filip Pizlo
2016-11-07 16:22 PST, Filip Pizlo
2016-11-07 18:27 PST, Filip Pizlo
2016-11-08 11:49 PST, Filip Pizlo
2016-11-08 15:03 PST, Filip Pizlo
2016-11-08 15:38 PST, Filip Pizlo
2016-11-08 17:21 PST, Filip Pizlo
2016-11-09 19:51 PST, Filip Pizlo
2016-11-09 20:27 PST, Filip Pizlo
2016-11-10 17:47 PST, Filip Pizlo
2016-11-11 11:27 PST, Filip Pizlo
2016-11-11 13:07 PST, Filip Pizlo
2016-11-12 09:15 PST, Filip Pizlo
2016-11-12 09:41 PST, Filip Pizlo
2016-11-12 11:35 PST, Filip Pizlo
2016-11-13 19:34 PST, Filip Pizlo
2016-11-14 11:38 PST, Filip Pizlo
2016-11-14 13:06 PST, Build Bot
2016-11-14 14:03 PST, Filip Pizlo
2016-11-14 14:07 PST, Filip Pizlo
2016-11-14 14:14 PST, Filip Pizlo