- Revision
- 153208
- Author
- oli...@apple.com
- Date
- 2013-07-24 21:02:00 -0700 (Wed, 24 Jul 2013)
Log Message
fourthTier: Clean up AbstractValue
https://bugs.webkit.org/show_bug.cgi?id=117217
Reviewed by Oliver Hunt.
This started as an attempt to make it so that when AbstractValue becomes empty,
its m_type always becomes SpecNone. I wanted this to happen naturally. That turns
out to be basically impossible, since AbstractValue is a set that is dynamically
computed from the intersection of several internal sets: so the value becomes
empty when any of the sets go empty. It's OK if we're imprecise here because it's
always safe for the AbstractValue to seem to overapproximate the set of values
that we see. So I mostly gave up on cleaning up that aspect of AbstractValue. But
while trying to make this happen, I encountered two bugs:
- filterValueByType() ignores the case when m_type contravenes m_value. Namely,
we might filter the AbstractValue against a SpeculatedType leading to m_value
becoming inconsistent with the new m_type. This change fixes that case. This
wasn't a symptomatic bug but it was a silly oversight.
- filterFuturePossibleStructure() was never right. The one call to this method,
in filter(Graph&, const StructureSet&), assumed that the previous notions of
what structures the value could have in the future were still relevant. This
could lead to a bug where we:
1) CheckStructure(@foo, S1)
Where S1 has a valid watchpoint. Now @foo's abstract value will have current
and future structure = S1.
2) Clobber the world.
Now @foo's abstract value will have current structure = TOP, and future
possible structure = S1.
3) CheckStructure(@foo, S2)
Now @foo's abstract value will have current structure = S2 and future
possible structure = S1 intersect S2 = BOTTOM.
Now we will think that any subsequent watchpoint on @foo is valid because the
value is effectively BOTTOM. That would only be correct if we had actually set
a watchpoint on S1. If we had done so, then (3) would only pass (i.e. @foo
would only have structure S2) if S1's watchpoint fired, in which case (3)
wouldn't have been reachable. But we didn't actually set a watchpoint on S1:
we just observed that we *could* have set the watchpoint. Hence future possible
structure should only be set to either the known structure at compile-time, or
it should be the structure we just checked; in both cases it should only be set
if the structure is watchable.
Then, in addition to all of this, I changed AbstractValue's filtering methods to
call clear() if the AbstractValue is effectively clear. This is just meant to
simplify the recognition of truly empty AbstractValues, but doesn't actually have
any other implications.
* bytecode/StructureSet.h:
(JSC::StructureSet::dump):
* dfg/DFGAbstractValue.cpp:
(JSC::DFG::AbstractValue::filter):
(DFG):
(JSC::DFG::AbstractValue::filterArrayModes):
(JSC::DFG::AbstractValue::filterValueByType):
(JSC::DFG::AbstractValue::filterArrayModesByType):
(JSC::DFG::AbstractValue::shouldBeClear):
(JSC::DFG::AbstractValue::normalizeClarity):
(JSC::DFG::AbstractValue::checkConsistency):
* dfg/DFGAbstractValue.h:
(JSC::DFG::AbstractValue::isClear):
(AbstractValue):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (153207 => 153208)
--- trunk/Source/_javascript_Core/ChangeLog 2013-07-25 04:01:59 UTC (rev 153207)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-07-25 04:02:00 UTC (rev 153208)
@@ -1,3 +1,81 @@
+2013-06-05 Filip Pizlo <fpi...@apple.com>
+
+ Unreviewed, fix release build.
+
+ * interpreter/Interpreter.cpp:
+ * jit/JITStubs.cpp:
+
+2013-06-04 Filip Pizlo <fpi...@apple.com>
+
+ fourthTier: Clean up AbstractValue
+ https://bugs.webkit.org/show_bug.cgi?id=117217
+
+ Reviewed by Oliver Hunt.
+
+ This started as an attempt to make it so that when AbstractValue becomes empty,
+ its m_type always becomes SpecNone. I wanted this to happen naturally. That turns
+ out to be basically impossible, since AbstractValue is a set that is dynamically
+ computed from the intersection of several internal sets: so the value becomes
+ empty when any of the sets go empty. It's OK if we're imprecise here because it's
+ always safe for the AbstractValue to seem to overapproximate the set of values
+ that we see. So I mostly gave up on cleaning up that aspect of AbstractValue. But
+ while trying to make this happen, I encountered two bugs:
+
+ - filterValueByType() ignores the case when m_type contravenes m_value. Namely,
+ we might filter the AbstractValue against a SpeculatedType leading to m_value
+ becoming inconsistent with the new m_type. This change fixes that case. This
+ wasn't a symptomatic bug but it was a silly oversight.
+
+ - filterFuturePossibleStructure() was never right. The one call to this method,
+ in filter(Graph&, const StructureSet&), assumed that the previous notions of
+ what structures the value could have in the future were still relevant. This
+ could lead to a bug where we:
+
+ 1) CheckStructure(@foo, S1)
+
+ Where S1 has a valid watchpoint. Now @foo's abstract value will have current
+ and future structure = S1.
+
+ 2) Clobber the world.
+
+ Now @foo's abstract value will have current structure = TOP, and future
+ possible structure = S1.
+
+ 3) CheckStructure(@foo, S2)
+
+ Now @foo's abstract value will have current structure = S2 and future
+ possible structure = S1 intersect S2 = BOTTOM.
+
+ Now we will think that any subsequent watchpoint on @foo is valid because the
+ value is effectively BOTTOM. That would only be correct if we had actually set
+ a watchpoint on S1. If we had done so, then (3) would only pass (i.e. @foo
+ would only have structure S2) if S1's watchpoint fired, in which case (3)
+ wouldn't have been reachable. But we didn't actually set a watchpoint on S1:
+ we just observed that we *could* have set the watchpoint. Hence future possible
+ structure should only be set to either the known structure at compile-time, or
+ it should be the structure we just checked; in both cases it should only be set
+ if the structure is watchable.
+
+ Then, in addition to all of this, I changed AbstractValue's filtering methods to
+ call clear() if the AbstractValue is effectively clear. This is just meant to
+ simplify the recognition of truly empty AbstractValues, but doesn't actually have
+ any other implications.
+
+ * bytecode/StructureSet.h:
+ (JSC::StructureSet::dump):
+ * dfg/DFGAbstractValue.cpp:
+ (JSC::DFG::AbstractValue::filter):
+ (DFG):
+ (JSC::DFG::AbstractValue::filterArrayModes):
+ (JSC::DFG::AbstractValue::filterValueByType):
+ (JSC::DFG::AbstractValue::filterArrayModesByType):
+ (JSC::DFG::AbstractValue::shouldBeClear):
+ (JSC::DFG::AbstractValue::normalizeClarity):
+ (JSC::DFG::AbstractValue::checkConsistency):
+ * dfg/DFGAbstractValue.h:
+ (JSC::DFG::AbstractValue::isClear):
+ (AbstractValue):
+
2013-06-04 Mark Lam <mark....@apple.com>
The DFG JIT should populate frame bytecodeOffsets on OSR exit.
Modified: trunk/Source/_javascript_Core/bytecode/StructureSet.h (153207 => 153208)
--- trunk/Source/_javascript_Core/bytecode/StructureSet.h 2013-07-25 04:01:59 UTC (rev 153207)
+++ trunk/Source/_javascript_Core/bytecode/StructureSet.h 2013-07-25 04:02:00 UTC (rev 153208)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2011 Apple Inc. All rights reserved.
+ * Copyright (C) 2011, 2013 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -30,6 +30,7 @@
#include "SpeculatedType.h"
#include "Structure.h"
#include <stdio.h>
+#include <wtf/CommaPrinter.h>
#include <wtf/Vector.h>
namespace JSC {
@@ -161,15 +162,13 @@
return true;
}
- void dump(FILE* out)
+ void dump(PrintStream& out) const
{
- fprintf(out, "[");
- for (size_t i = 0; i < m_structures.size(); ++i) {
- if (i)
- fprintf(out, ", ");
- fprintf(out, "%p", m_structures[i]);
- }
- fprintf(out, "]");
+ CommaPrinter comma;
+ out.print("[");
+ for (size_t i = 0; i < m_structures.size(); ++i)
+ out.print(comma, RawPointer(m_structures[i]));
+ out.print("]");
}
private:
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractValue.cpp (153207 => 153208)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractValue.cpp 2013-07-25 04:01:59 UTC (rev 153207)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractValue.cpp 2013-07-25 04:02:00 UTC (rev 153208)
@@ -79,7 +79,7 @@
m_arrayModes = asArrayModes(structure->indexingType());
m_type = speculationFromStructure(structure);
m_value = JSValue();
-
+
checkConsistency();
}
@@ -88,28 +88,58 @@
// FIXME: This could be optimized for the common case of m_type not
// having structures, array modes, or a specific value.
// https://bugs.webkit.org/show_bug.cgi?id=109663
+
m_type &= other.speculationFromStructures();
m_arrayModes &= other.arrayModesFromStructures();
m_currentKnownStructure.filter(other);
- if (m_currentKnownStructure.isClear())
- m_futurePossibleStructure.clear();
- else if (m_currentKnownStructure.hasSingleton())
- filterFuturePossibleStructure(graph, m_currentKnownStructure.singleton());
-
+
// It's possible that prior to the above two statements we had (Foo, TOP), where
// Foo is a SpeculatedType that is disjoint with the passed StructureSet. In that
// case, we will now have (None, [someStructure]). In general, we need to make
// sure that new information gleaned from the SpeculatedType needs to be fed back
// into the information gleaned from the StructureSet.
m_currentKnownStructure.filter(m_type);
- m_futurePossibleStructure.filter(m_type);
+
+ if (m_currentKnownStructure.hasSingleton())
+ setFuturePossibleStructure(graph, m_currentKnownStructure.singleton());
filterArrayModesByType();
filterValueByType();
-
+ normalizeClarity();
+
checkConsistency();
}
+void AbstractValue::filterArrayModes(ArrayModes arrayModes)
+{
+ ASSERT(arrayModes);
+
+ m_type &= SpecCell;
+ m_arrayModes &= arrayModes;
+ normalizeClarity();
+
+ checkConsistency();
+}
+
+void AbstractValue::filter(SpeculatedType type)
+{
+ if (type == SpecTop)
+ return;
+ m_type &= type;
+
+ // It's possible that prior to this filter() call we had, say, (Final, TOP), and
+ // the passed type is Array. At this point we'll have (None, TOP). The best way
+ // to ensure that the structure filtering does the right thing is to filter on
+ // the new type (None) rather than the one passed (Array).
+ m_currentKnownStructure.filter(m_type);
+ m_futurePossibleStructure.filter(m_type);
+ filterArrayModesByType();
+ filterValueByType();
+ normalizeClarity();
+
+ checkConsistency();
+}
+
void AbstractValue::setFuturePossibleStructure(Graph& graph, Structure* structure)
{
if (graph.watchpoints().isStillValid(structure->transitionWatchpointSet()))
@@ -118,12 +148,78 @@
m_futurePossibleStructure.makeTop();
}
-void AbstractValue::filterFuturePossibleStructure(Graph& graph, Structure* structure)
+void AbstractValue::filterValueByType()
{
- if (graph.watchpoints().isStillValid(structure->transitionWatchpointSet()))
- m_futurePossibleStructure.filter(StructureAbstractValue(structure));
+ // We could go further, and ensure that if the futurePossibleStructure contravenes
+ // the value, then we could clear both of those things. But that's unlikely to help
+ // in any realistic scenario, so we don't do it. Simpler is better.
+
+ if (!!m_type) {
+ // The type is still non-empty. It may be that the new type renders
+ // the value empty because it contravenes the constant value we had.
+ if (m_value && !validateType(m_value))
+ clear();
+ return;
+ }
+
+ // The type has been rendered empty. That means that the value must now be invalid,
+ // as well.
+ ASSERT(!m_value || !validateType(m_value));
+ m_value = JSValue();
}
+void AbstractValue::filterArrayModesByType()
+{
+ if (!(m_type & SpecCell))
+ m_arrayModes = 0;
+ else if (!(m_type & ~SpecArray))
+ m_arrayModes &= ALL_ARRAY_ARRAY_MODES;
+ else if (!(m_type & SpecArray))
+ m_arrayModes &= ALL_NON_ARRAY_ARRAY_MODES;
+}
+
+bool AbstractValue::shouldBeClear() const
+{
+ if (m_type == SpecNone)
+ return true;
+
+ if (!(m_type & ~SpecCell)
+ && (!m_arrayModes
+ || m_currentKnownStructure.isClear()))
+ return true;
+
+ return false;
+}
+
+void AbstractValue::normalizeClarity()
+{
+ // It's useful to be able to quickly check if an abstract value is clear.
+ // This normalizes everything to make that easy.
+
+ if (shouldBeClear())
+ clear();
+}
+
+void AbstractValue::checkConsistency() const
+{
+ if (!(m_type & SpecCell)) {
+ ASSERT(m_currentKnownStructure.isClear());
+ ASSERT(m_futurePossibleStructure.isClear());
+ ASSERT(!m_arrayModes);
+ }
+
+ if (isClear())
+ ASSERT(!m_value);
+
+ if (!!m_value)
+ ASSERT(mergeSpeculations(m_type, speculationFromValue(m_value)) == m_type);
+
+ // Note that it's possible for a prediction like (Final, []). This really means that
+ // the value is bottom and that any code that uses the value is unreachable. But
+ // we don't want to get pedantic about this as it would only increase the computational
+ // complexity of the code.
+}
+
void AbstractValue::dump(PrintStream& out) const
{
out.print(
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractValue.h (153207 => 153208)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractValue.h 2013-07-25 04:01:59 UTC (rev 153207)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractValue.h 2013-07-25 04:02:00 UTC (rev 153208)
@@ -57,13 +57,7 @@
checkConsistency();
}
- bool isClear() const
- {
- bool result = m_type == SpecNone && !m_arrayModes && m_currentKnownStructure.isClear() && m_futurePossibleStructure.isClear();
- if (result)
- ASSERT(!m_value);
- return result;
- }
+ bool isClear() const { return m_type == SpecNone; }
void makeTop()
{
@@ -190,36 +184,9 @@
void filter(Graph&, const StructureSet&);
- void filterArrayModes(ArrayModes arrayModes)
- {
- ASSERT(arrayModes);
-
- m_type &= SpecCell;
- m_arrayModes &= arrayModes;
-
- // I could do more fancy filtering here. But it probably won't make any difference.
-
- checkConsistency();
- }
+ void filterArrayModes(ArrayModes arrayModes);
- void filter(SpeculatedType type)
- {
- if (type == SpecTop)
- return;
- m_type &= type;
-
- // It's possible that prior to this filter() call we had, say, (Final, TOP), and
- // the passed type is Array. At this point we'll have (None, TOP). The best way
- // to ensure that the structure filtering does the right thing is to filter on
- // the new type (None) rather than the one passed (Array).
- m_currentKnownStructure.filter(m_type);
- m_futurePossibleStructure.filter(m_type);
-
- filterArrayModesByType();
- filterValueByType();
-
- checkConsistency();
- }
+ void filter(SpeculatedType type);
void filterByValue(JSValue value)
{
@@ -280,25 +247,7 @@
return 0;
}
- void checkConsistency() const
- {
- if (!(m_type & SpecCell)) {
- ASSERT(m_currentKnownStructure.isClear());
- ASSERT(m_futurePossibleStructure.isClear());
- ASSERT(!m_arrayModes);
- }
-
- if (isClear())
- ASSERT(!m_value);
-
- if (!!m_value)
- ASSERT(mergeSpeculations(m_type, speculationFromValue(m_value)) == m_type);
-
- // Note that it's possible for a prediction like (Final, []). This really means that
- // the value is bottom and that any code that uses the value is unreachable. But
- // we don't want to get pedantic about this as it would only increase the computational
- // complexity of the code.
- }
+ void checkConsistency() const;
void dump(PrintStream&) const;
@@ -314,7 +263,7 @@
// y = x.f;
//
// Where x will later have a new property added to it, 'g'. Because of the
- // known but not-yet-executed property addition, x's currently structure will
+ // known but not-yet-executed property addition, x's current structure will
// not be watchpointable; hence we have no way of statically bounding the set
// of possible structures that x may have if a clobbering event happens. So,
// x's m_currentKnownStructure will be whatever structure we check to get
@@ -410,45 +359,12 @@
}
void setFuturePossibleStructure(Graph&, Structure* structure);
- void filterFuturePossibleStructure(Graph&, Structure* structure);
- // We could go further, and ensure that if the futurePossibleStructure contravenes
- // the value, then we could clear both of those things. But that's unlikely to help
- // in any realistic scenario, so we don't do it. Simpler is better.
- void filterValueByType()
- {
- if (!!m_type) {
- // The type is still non-empty. This implies that regardless of what filtering
- // was done, we either didn't have a value to begin with, or that value is still
- // valid.
- ASSERT(!m_value || validateType(m_value));
- return;
- }
-
- // The type has been rendered empty. That means that the value must now be invalid,
- // as well.
- ASSERT(!m_value || !validateType(m_value));
- m_value = JSValue();
- }
+ void filterValueByType();
+ void filterArrayModesByType();
- void filterArrayModesByType()
- {
- if (!(m_type & SpecCell))
- m_arrayModes = 0;
- else if (!(m_type & ~SpecArray))
- m_arrayModes &= ALL_ARRAY_ARRAY_MODES;
-
- // NOTE: If m_type doesn't have SpecArray set, that doesn't mean that the
- // array modes have to be a subset of ALL_NON_ARRAY_ARRAY_MODES, since
- // in the speculated type type-system, RegExpMatchesArry and ArrayPrototype
- // are Otherobj (since they are not *exactly* JSArray) but in the ArrayModes
- // type system they are arrays (since they expose the magical length
- // property and are otherwise allocated using array allocation). Hence the
- // following would be wrong:
- //
- // if (!(m_type & SpecArray))
- // m_arrayModes &= ALL_NON_ARRAY_ARRAY_MODES;
- }
+ bool shouldBeClear() const;
+ void normalizeClarity();
};
} } // namespace JSC::DFG
