Bug 164175 – [DOMJIT][JSC] Allow DFG GetById optimization for impure objects with NewImpurePropertyFiresWatchpoints

• Component: JavaScriptCore
• Status: NEW

[Yusuke Suzuki]

The most frequently used one is `document`.

[Yusuke Suzuki]

The challenge is that impure property's watchpointset is managed by the hash table. And it will be removed once it is fired.

Super simple idea is that,

1. Guard this hash table with lock
2. When registering watchpoint to this watchpointset in DFG side, we first hold this watchpointset with Ref in the DFG side. DesiredWatchpoints will hold impure watchpointsets with Ref<WatchpointSet>

[Yusuke Suzuki]

No, if AccessCase correctly holds the watchpointset, lock is not necessary.

[Yusuke Suzuki]

Start working on it

[Yusuke Suzuki]

Created attachment 294564 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 294566 [details]
Patch

WIP

[Filip Pizlo]

Comment on attachment 294566 [details]
Patch

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

I like where this is going.

How do you plan to handle thrashing?  Currently the only watchpoints that a GetByIdStatus would tell the DFG to set are watchpoints that we know very well how to set without getting into trouble.  There's a ton of infrastructure to protect us from repeatedly recompiling due to structure transition watchpoints.  But this watchpoint is different.  We allow this watchpoint to repeatedly clear inline caches because it's cheap to repatch an inline cache.  It's not cheap to recompile something with the DFG or FTL.  So, we need some better defense.  The DFG recompilation infrastructure will not really know why the code was jettisoned - I mean, it'll have some kind of debug string, but that's not really enough.  The way we handle this with most other structures is that they are monotonic.  Once they are fired, they stay invalidated forever, so if the DFG recompiles then it will not make the same mistake again - the DFG always has a safe path for when the watchpoint is already invalid.  But these impure watchpoints are not designed to work that way!  They simply get deleted when they are fired.  Afterwards nobody knows that they had ever been there.  This is great for inline caches, since it's OK for them to try as many times as they want.  But the DFG needs to know somehow that it should not make the same mistake again.

Here are possible ways to do this:
- Have each StructureStubInfo track an extra bit for when that watchpoint got fired.  We would make this work by having StructureStubClearingWatchpoint know about when it's being used for impure properties, and in that case, it will set the bit.  Then each IC will remember if it had ever been messed with.  Maybe this is good enough.
- Have VM remember when we fire impure watchpoints on a given name.  We would probably want to clear that remembered set periodically (stupid but effective: GC clears the set if it has not done so in more than 1 second).  This will accelerate convergence when combined with the first approach.  A nice variant of this is that you only remember when addImpureProperty fired a watchpoint set that was being watched.  This way, we don't remember anything about impure properties that don't overlap with the properties that ICs are caching.
- There could be a bit in the baseline CodeBlock that tells if we had ever seen an impure property watchpoint fire, or if it had ever caused us to recompile.

I think that the first approach is mandatory and the other two are optional.

> Source/JavaScriptCore/bytecode/ComplexGetStatus.cpp:45
> +        if (!impurePropertySet && impurePropertySet->hasBeenInvalidated())

I think you meant ||

> Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:111
> +    if (m_impurePropertySet != other.m_impurePropertySet)
> +        return false;
> +

If only it was a set of sets!  Like m_extraWatchpoints or something.

I see that you are renaming m_additionalSet to m_impurePropertySet.  I would have hesitated before doing that.  I guess calling it m_impurePropertySet is a signal from AccessCase to GetByIdStatus/ComplexGetStatus/etc that we had done the impure property optimization.  But we don't really need to convey that signal, since the Structure will tell us this already.  The DFG could still know that the impure thing is in play, and make decisions based on that, just by looking into the structure.  This is nice, since that means that if we ever added another reason for an "additionalSet", that was unrelated to impure properties, then we may get away with simply teaching AccessCase about it and trusting that this will magically make the DFG watch this watchpoint.

In this world, the meaning of the additionalSet is very concise: it just means that you've got to watch that watchpoint!  You don't have to know anything more.  Such an easy mental model.

Does this make sense, or am I overlooking something?

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:297
> +    result->initializeImpurePropertySetIfNecessary(vm, ident);

By the way, in my world, I would have named this method just as you did.  But in my world, only this method would have to know about the exact relationship between impure properties and the watchpoint set.

[Yusuke Suzuki]

Thanks!!!

(In reply to comment #6)
> Comment on attachment 294566 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=294566&action=review
> 
> I like where this is going.
> 
> How do you plan to handle thrashing?  Currently the only watchpoints that a
> GetByIdStatus would tell the DFG to set are watchpoints that we know very
> well how to set without getting into trouble.  There's a ton of
> infrastructure to protect us from repeatedly recompiling due to structure
> transition watchpoints.  But this watchpoint is different.  We allow this
> watchpoint to repeatedly clear inline caches because it's cheap to repatch
> an inline cache.  It's not cheap to recompile something with the DFG or FTL.
> So, we need some better defense.  The DFG recompilation infrastructure will
> not really know why the code was jettisoned - I mean, it'll have some kind
> of debug string, but that's not really enough.  The way we handle this with
> most other structures is that they are monotonic.  Once they are fired, they
> stay invalidated forever, so if the DFG recompiles then it will not make the
> same mistake again - the DFG always has a safe path for when the watchpoint
> is already invalid.  But these impure watchpoints are not designed to work
> that way!  They simply get deleted when they are fired.  Afterwards nobody
> knows that they had ever been there.  This is great for inline caches, since
> it's OK for them to try as many times as they want.  But the DFG needs to
> know somehow that it should not make the same mistake again.

Right. Impure watchpoint is removed from the map when impure property is added.
So, watchpointSet->hasBeenInvalidated() check is not enough.

> 
> Here are possible ways to do this:
> - Have each StructureStubInfo track an extra bit for when that watchpoint
> got fired.  We would make this work by having
> StructureStubClearingWatchpoint know about when it's being used for impure
> properties, and in that case, it will set the bit.  Then each IC will
> remember if it had ever been messed with.  Maybe this is good enough.

Sounds nice. It should work.

> - Have VM remember when we fire impure watchpoints on a given name.  We
> would probably want to clear that remembered set periodically (stupid but
> effective: GC clears the set if it has not done so in more than 1 second). 
> This will accelerate convergence when combined with the first approach.  A
> nice variant of this is that you only remember when addImpureProperty fired
> a watchpoint set that was being watched.  This way, we don't remember
> anything about impure properties that don't overlap with the properties that
> ICs are caching.

> - There could be a bit in the baseline CodeBlock that tells if we had ever
> seen an impure property watchpoint fire, or if it had ever caused us to
> recompile.
> 
> I think that the first approach is mandatory and the other two are optional.

Agreed. The first one must be done.
Second, and third ones can be done.
I think implementing the first one is good for the first step.
If we figured out that the first one is not enough for the performance in the benchmarks or so, we should include the second and third ones in the same patch.

> 
> > Source/JavaScriptCore/bytecode/ComplexGetStatus.cpp:45
> > +        if (!impurePropertySet && impurePropertySet->hasBeenInvalidated())
> 
> I think you meant ||

Oops, right.

> 
> > Source/JavaScriptCore/bytecode/GetByIdVariant.cpp:111
> > +    if (m_impurePropertySet != other.m_impurePropertySet)
> > +        return false;
> > +
> 
> If only it was a set of sets!  Like m_extraWatchpoints or something.
> 
> I see that you are renaming m_additionalSet to m_impurePropertySet.  I would
> have hesitated before doing that.  I guess calling it m_impurePropertySet is
> a signal from AccessCase to GetByIdStatus/ComplexGetStatus/etc that we had
> done the impure property optimization.  But we don't really need to convey
> that signal, since the Structure will tell us this already.  The DFG could
> still know that the impure thing is in play, and make decisions based on
> that, just by looking into the structure.  This is nice, since that means
> that if we ever added another reason for an "additionalSet", that was
> unrelated to impure properties, then we may get away with simply teaching
> AccessCase about it and trusting that this will magically make the DFG watch
> this watchpoint.
> 
> In this world, the meaning of the additionalSet is very concise: it just
> means that you've got to watch that watchpoint!  You don't have to know
> anything more.  Such an easy mental model.
> 
> Does this make sense, or am I overlooking something?

It sounds very nice.

> 
> > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:297
> > +    result->initializeImpurePropertySetIfNecessary(vm, ident);
> 
> By the way, in my world, I would have named this method just as you did. 
> But in my world, only this method would have to know about the exact
> relationship between impure properties and the watchpoint set.

Yeah, if we just hold extraWatchpoints, only this place should know about impure thing.

[Yusuke Suzuki]

Created attachment 294924 [details]
Patch

WIP, start holding extraWatchpoints

[Yusuke Suzuki]

fun fact: Once GetByOffset for document is landed, Dromaeo's query test becomes more & more meaningless.