[webkit-reviews] review denied: [Bug 125377] REGRESSION: 2x regression on Dromaeo DOM query tests : [Attachment 219042] New watchpoint approach work in progress

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 11 23:44:44 PST 2013


Filip Pizlo <fpizlo at apple.com> has denied  review:
Bug 125377: REGRESSION: 2x regression on Dromaeo DOM query tests
https://bugs.webkit.org/show_bug.cgi?id=125377

Attachment 219042: New watchpoint approach work in progress
https://bugs.webkit.org/attachment.cgi?id=219042&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=219042&action=review


> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.cpp:58
> +    if (m_watchpointSetToUnregisterAtDestruction)
> +	  
m_codeBlock->vm()->unregisterWatchpointSetForImpureProperty(m_propertyNameForUn
registeringWatchpointSet, *m_watchpointSetToUnregisterAtDestruction);
>  }

This isn't needed.

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:34
> +#include "Identifier.h"

This isn't needed.

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:96
> +    void unregisterWatchpointSetAtDestruction(const Identifier&
propertyName, InlineWatchpointSet& watchpointSet)
> +    {
> +	   ASSERT(!m_watchpointSetToUnregisterAtDestruction);
> +	   m_watchpointSetToUnregisterAtDestruction = &watchpointSet;
> +	   // FIXME: We should store AtomicString once we've merged it with
Identifier.
> +	   m_propertyNameForUnregisteringWatchpointSet = propertyName.string();

> +    }
> +

Get rid of this.

> Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.h:113
> +    InlineWatchpointSet* m_watchpointSetToUnregisterAtDestruction;
> +    String m_propertyNameForUnregisteringWatchpointSet;

Get rid of this.

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:41
> +#include <wtf/text/WTFString.h>

Get rid of this.

> Source/JavaScriptCore/bytecode/StructureStubInfo.h:224
> +
> +    void unregisterWatchpointSetAtDestruction(const Identifier&
propertyName, InlineWatchpointSet& watchpointSet)
> +    {
> +	   watchpoints->unregisterWatchpointSetAtDestruction(propertyName,
watchpointSet);
> +    }

You don't need this.

> Source/JavaScriptCore/jit/Repatch.cpp:260
> +	   if
(prototypeStructure->typeInfo().newImpurePropertyFiresWatchpoints()) {
> +	       InlineWatchpointSet& set =
currStructure->transitionWatchpointSet();
> +	       stubInfo.unregisterWatchpointSetAtDestruction(propertyName,
set);
> +	       vm->registerWatchpointSetForImpureProperty(propertyName, set);
> +	   }

Don't use the transitionWatchpointSet().  That set is for transitions.	You're
not doing transitions.

Instead you should say:

if (prototypeStructure->typeInfo().newImpurePropertyFiresWatchpoints()) {
    Watchpoint* watchpoint = stubInfo.addWatchpoint(codeBlock);
    vm->registerWatchpointForImpureProperty(propertyName, watchpoint);
}

What you're doing is basically wrong, or at least highly circuitous.  It's
wrong because:

- A watchpoint set should be associate with some conceptual event.  The
structure's transition watchpoint set
(currStructure->transitionWatchpointSet()) corresponds to a transition.  It's
fired whenever there is a transition, and people register watchpoints on
transitions with that watchpoint set.  You're reusing that set for your own
purposes, which don't involve transitions.  That's just plain wrong.  You're
making it so that if an impure property gets added, a bunch of listeners will
think there had been a transition (even though no transition occurred).  You'll
also fire the impure property thing whenever a transition occurs, even though a
transition doesn't necessarily imply an impure property.  You're conflating
unrelated things.

- As a result of your reuse of the transition watchpoint set, you create a mess
for yourself trying to manage the lifecycle of the transition watchpoint set. 
It's true that this is hard, but it's only hard because you're using an
existing set instead of creating a new one.

- You shouldn't be using an InlineWatchpointSet at all.

Even if what you were doing wasn't outright wrong, it's clear that you're
creating a *ton* of infrastructure for lifecycle management.  That
infrastructure already exists.	It's called a watchpoint set.  The reason why
you're unable to leverage that existing infrastructure is that you're reusing
an existing watchpoint set which has its lifecycle tied to a GC'd object.  So,
yeah, that's going to be hard.	But you don't need to use that watchpoint set. 
Just create your own watchpoint set and you won't have to write half the code
in this patch.

> Source/JavaScriptCore/runtime/VM.cpp:795
> +void VM::registerWatchpointSetForImpureProperty(const Identifier&
propertyName, InlineWatchpointSet& watchpointSet)

The signature of this method should be:

void VM::registerWatchpointForImpureProperty(const Identifier& propertyName,
Watchpoint* watchpoint)

> Source/JavaScriptCore/runtime/VM.cpp:798
> +    auto result =
m_impurePropertyWatchpointSetSets.add(propertyName.string(),
HashSet<InlineWatchpointSet*>());
> +    result.iterator->value.add(&watchpointSet);

This should check if the map has a WatchpointSet* for the property name.  It
should create one if one didn't already exit.  Then it should add the
watchpoint to that set.

Even if there was a way to make your approach work, it just feels awfully
wrong: you have a map of sets of sets!	The whole point of a watchpoint set is
to be a set of watchpoints.  You're creating a set of watchpoint sets, where
those sets are borrowed from some unrelated Structure*, and then you're placing
watchpoints into those sets. :-/  That's highly unusual.

> Source/JavaScriptCore/runtime/VM.cpp:809
> +void VM::unregisterWatchpointSetForImpureProperty(const String&
propertyName, InlineWatchpointSet& watchpointSet)
> +{
> +    auto result = m_impurePropertyWatchpointSetSets.find(propertyName);
> +    ASSERT(result != m_impurePropertyWatchpointSetSets.end());
> +    if (result->value.size() == 1)
> +	   m_impurePropertyWatchpointSetSets.remove(result);
> +    else
> +	   result->value.remove(&watchpointSet);
> +}

Kill this method.

> Source/JavaScriptCore/runtime/VM.cpp:817
> +    for (auto it = result->value.begin(), end = result->value.end(); it !=
end; ++it)
> +	   (*it)->fireAll();

You don't need this loop, since result->value should be a
RefPtr<WatchpointSet>.	Then you call fireAll().  Then you should do
m_impurePropertyWatchpointSets.remove(propertyName).  The reason for this is
that WatchpointSets are one-shot.  So, after you fire it, it invalidates all
watchpoints and then invalidates itself.  fireAll() is idempotent: fireAll() a
second time will do nothing.

Also removing it saves memory.

> Source/JavaScriptCore/runtime/VM.h:523
> +	   HashMap<String, HashSet<InlineWatchpointSet*>>
m_impurePropertyWatchpointSetSets;

This should be:

HashMap<String, RefPtr<WatchpointSet>> m_impurePropertyWatchpointSets;


More information about the webkit-reviews mailing list