Title: [186745] trunk/Source/_javascript_Core
- Revision
- 186745
- Author
- [email protected]
- Date
- 2015-07-12 19:16:17 -0700 (Sun, 12 Jul 2015)
Log Message
Watchpoints should be removed from their owning WatchpointSet before they are fired
https://bugs.webkit.org/show_bug.cgi?id=146895
Reviewed by Sam Weinig.
This simplifies the WatchpointSet API by making it so that when Watchpoint::fire() is
called, the Watchpoint is no longer in the set. This means that you don't have to remember
to remove it from the set's list (currently we do that implicitly as part of whatever
any particular Watchpoint::fireInternal() does), and you can now write adaptive
watchpoints that re-add themselves to a different set if they determine that the thing
they are actually watching is still intact but now needs to be watched in a different way
(like watching for whether some singleton object has a property of some name).
* bytecode/Watchpoint.cpp:
(JSC::Watchpoint::~Watchpoint): Add a comment about why this is necessary.
(JSC::Watchpoint::fire): Make this out-of-line, private, and make it assert that we're no longer on the list.
(JSC::WatchpointSet::fireAllWatchpoints): Make this remove the watchpoint from the list before firing it.
* bytecode/Watchpoint.h:
(JSC::Watchpoint::fire): Deleted. I moved this to Watchpoint.cpp.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (186744 => 186745)
--- trunk/Source/_javascript_Core/ChangeLog 2015-07-13 01:39:36 UTC (rev 186744)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-07-13 02:16:17 UTC (rev 186745)
@@ -1,3 +1,25 @@
+2015-07-12 Filip Pizlo <[email protected]>
+
+ Watchpoints should be removed from their owning WatchpointSet before they are fired
+ https://bugs.webkit.org/show_bug.cgi?id=146895
+
+ Reviewed by Sam Weinig.
+
+ This simplifies the WatchpointSet API by making it so that when Watchpoint::fire() is
+ called, the Watchpoint is no longer in the set. This means that you don't have to remember
+ to remove it from the set's list (currently we do that implicitly as part of whatever
+ any particular Watchpoint::fireInternal() does), and you can now write adaptive
+ watchpoints that re-add themselves to a different set if they determine that the thing
+ they are actually watching is still intact but now needs to be watched in a different way
+ (like watching for whether some singleton object has a property of some name).
+
+ * bytecode/Watchpoint.cpp:
+ (JSC::Watchpoint::~Watchpoint): Add a comment about why this is necessary.
+ (JSC::Watchpoint::fire): Make this out-of-line, private, and make it assert that we're no longer on the list.
+ (JSC::WatchpointSet::fireAllWatchpoints): Make this remove the watchpoint from the list before firing it.
+ * bytecode/Watchpoint.h:
+ (JSC::Watchpoint::fire): Deleted. I moved this to Watchpoint.cpp.
+
2015-07-10 Filip Pizlo <[email protected]>
DFG::DesiredWatchpoints should accept WatchpointSetType's that aren't necessarily pointers
Modified: trunk/Source/_javascript_Core/bytecode/Watchpoint.cpp (186744 => 186745)
--- trunk/Source/_javascript_Core/bytecode/Watchpoint.cpp 2015-07-13 01:39:36 UTC (rev 186744)
+++ trunk/Source/_javascript_Core/bytecode/Watchpoint.cpp 2015-07-13 02:16:17 UTC (rev 186745)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2012, 2013, 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-2015 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -38,10 +38,22 @@
Watchpoint::~Watchpoint()
{
- if (isOnList())
+ if (isOnList()) {
+ // This will happen if we get destroyed before the set fires. That's totally a valid
+ // possibility. For example:
+ //
+ // CodeBlock has a Watchpoint on transition from structure S1. The transition never
+ // happens, but the CodeBlock gets destroyed because of GC.
remove();
+ }
}
+void Watchpoint::fire(const FireDetail& detail)
+{
+ RELEASE_ASSERT(!isOnList());
+ fireInternal(detail);
+}
+
WatchpointSet::WatchpointSet(WatchpointState state)
: m_state(state)
, m_setIsNotEmpty(false)
@@ -86,8 +98,27 @@
void WatchpointSet::fireAllWatchpoints(const FireDetail& detail)
{
- while (!m_set.isEmpty())
- m_set.begin()->fire(detail);
+ while (!m_set.isEmpty()) {
+ Watchpoint* watchpoint = m_set.begin();
+ ASSERT(watchpoint->isOnList());
+
+ // Removing the Watchpoint before firing it makes it possible to implement watchpoints
+ // that add themselves to a different set when they fire. This kind of "adaptive"
+ // watchpoint can be used to track some semantic property that is more fine-graiend than
+ // what the set can convey. For example, we might care if a singleton object ever has a
+ // property called "foo". We can watch for this by checking if its Structure has "foo" and
+ // then watching its transitions. But then the watchpoint fires if any property is added.
+ // So, before the watchpoint decides to invalidate any code, it can check if it is
+ // possible to add itself to the transition watchpoint set of the singleton object's new
+ // Structure.
+ watchpoint->remove();
+ ASSERT(m_set.begin() != watchpoint);
+ ASSERT(!watchpoint->isOnList());
+
+ watchpoint->fire(detail);
+ // After we fire the watchpoint, the watchpoint pointer may be a dangling pointer. That's
+ // fine, because we have no use for the pointer anymore.
+ }
}
void InlineWatchpointSet::add(Watchpoint* watchpoint)
Modified: trunk/Source/_javascript_Core/bytecode/Watchpoint.h (186744 => 186745)
--- trunk/Source/_javascript_Core/bytecode/Watchpoint.h 2015-07-13 01:39:36 UTC (rev 186744)
+++ trunk/Source/_javascript_Core/bytecode/Watchpoint.h 2015-07-13 02:16:17 UTC (rev 186745)
@@ -63,6 +63,8 @@
const char* m_string;
};
+class WatchpointSet;
+
class Watchpoint : public BasicRawSentinelNode<Watchpoint> {
WTF_MAKE_NONCOPYABLE(Watchpoint);
WTF_MAKE_FAST_ALLOCATED;
@@ -73,10 +75,12 @@
virtual ~Watchpoint();
- void fire(const FireDetail& detail) { fireInternal(detail); }
-
protected:
virtual void fireInternal(const FireDetail&) = 0;
+
+private:
+ friend class WatchpointSet;
+ void fire(const FireDetail&);
};
enum WatchpointState {
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
