Bug 178051 – direct-construct-arity-mismatch.js can have GCs that take ~70ms if you force poly proto and disable generational GC

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Looks like there are insane amounts of weak handles.

[Filip Pizlo]

I bet it's the PrototypeMap.

Looking at it more, PrototypeMap is basically a memory leak. It won't show up as a leak because it's the kind of leak where all leaked objects are reachable. So, the leaks tool won't catch it. But it's a leak nonetheless.

It won't show up in GC logging, either. The GC maintains a really nice steady state of heap usage according to its own accounting.

But if you look at top -u, then dayum.  We're growing 10MB/sec in that test with forced poly proto.

The reason why it's a leak is that WeakGCMap is a leak.  We're sort of magically relying on things being removed from the map, but they never quite are, and so we just leak hella memory.

Another way that this shows up - and the way I caught it - is that the GC is spending all of its time in weak block processing.

Both problems are solved if PrototypeMap is wired directly into the GC. Because I need to do this to get tests to pass on my machine (currently this test times out), I'll just do it now.

[Filip Pizlo]

Oh man, we can make PrototypeMap so efficient.

First of all, it totally needs a MarkingConstraint. We want it to mark the structure if the keys are live. And we want it to remove anything if any part of the key is dead.

That will have the downstream effect of being super nice for inline caches, since it means structure stability if all objects with that structure die but if all of the code needed to spawn those structures again is still live.

[Filip Pizlo]

Wait - there's no way that WeakGCMap can be leaking in this case, since the keys are just raw pointers and the values are weak. Also, we prune during full collections.

So, I don't understand why we are leaking memory.

[Filip Pizlo]

I think I know why the GC sees so many weak references: https://bugs.webkit.org/show_bug.cgi?id=177909#c12

I still don't know why that causes a memory leak.

[Filip Pizlo]

So I guess the plan should be:

1) PrototypeMap probably doesn't need anything special to replace WeakGCMap::m_structures.  I was wrong to assume that.  It's probably correct the way that it is now, including the clearEmptyObjectStructureForPrototype thing.

2) PrototypeMap definitely needs to get rid of m_prototypes. That thing is nuts! It means that every single prototype instance created by poly proto will end up being stuffed into this ginormous WeakGCMap. For some dumb reason, this also leads to a memory leak - but the fundamental problem is that we should not be using a HashSet to answer if an object is a prototype.

[Filip Pizlo]

Aha!  Now I understand the leak.  The leak was fixed by Saam, but then the fix was rolled out, because that fix assumed that pruning was a requirement (it's only an optimization).  Details: https://bugs.webkit.org/show_bug.cgi?id=177952#c16

[Filip Pizlo]

Created attachment 323100 [details]
the patch

[Saam Barati]

Comment on attachment 323100 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323100&action=review

r=me

> Source/JavaScriptCore/runtime/IndexingType.h:76
> +static const IndexingType IndexingTypeMayBePrototype      = 0x80;

Nice I was thinking of exactly this. I agree this is much better than a large hashset

> Source/JavaScriptCore/runtime/PrototypeKey.h:45
> +        : m_inlineCapacity(1)

There’s no technical reason to do this, but I think it’s clearer to use UINT_MAC here. After initially reading it, I was worried that it was wrong, but then realized we had default initializers below

[Saam Barati]

(In reply to Filip Pizlo from comment #7)
> Aha!  Now I understand the leak.  The leak was fixed by Saam, but then the
> fix was rolled out, because that fix assumed that pruning was a requirement
> (it's only an optimization).  Details:
> https://bugs.webkit.org/show_bug.cgi?id=177952#c16

Right. It might be worth us adding a comment to WeakGCMap stating this is how it works so people don’t make the same bad assumption that I made.

[Filip Pizlo]

(In reply to Saam Barati from comment #9)
> Comment on attachment 323100 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323100&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/runtime/IndexingType.h:76
> > +static const IndexingType IndexingTypeMayBePrototype      = 0x80;
> 
> Nice I was thinking of exactly this. I agree this is much better than a
> large hashset
> 
> > Source/JavaScriptCore/runtime/PrototypeKey.h:45
> > +        : m_inlineCapacity(1)
> 
> There’s no technical reason to do this, but I think it’s clearer to use
> UINT_MAC here. 

You mean UINT_MAX?  Why is that clearer?

> After initially reading it, I was worried that it was wrong,
> but then realized we had default initializers below

[Filip Pizlo]

Created attachment 323113 [details]
patch for landing

[Saam Barati]

(In reply to Filip Pizlo from comment #11)
> (In reply to Saam Barati from comment #9)
> > Comment on attachment 323100 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=323100&action=review
> > 
> > r=me
> > 
> > > Source/JavaScriptCore/runtime/IndexingType.h:76
> > > +static const IndexingType IndexingTypeMayBePrototype      = 0x80;
> > 
> > Nice I was thinking of exactly this. I agree this is much better than a
> > large hashset
> > 
> > > Source/JavaScriptCore/runtime/PrototypeKey.h:45
> > > +        : m_inlineCapacity(1)
> > 
> > There’s no technical reason to do this, but I think it’s clearer to use
> > UINT_MAC here. 
> 
> You mean UINT_MAX?  Why is that clearer?
I think it’s more intuitive because it’s an invalid inlineCapacity
> 
> > After initially reading it, I was worried that it was wrong,
> > but then realized we had default initializers below

[Filip Pizlo]

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