- Revision
- 190529
- Author
- [email protected]
- Date
- 2015-10-02 16:09:33 -0700 (Fri, 02 Oct 2015)
Log Message
We should not add InferredTypeTables to old Structures
https://bugs.webkit.org/show_bug.cgi?id=149767
rdar://problem/22825526
Patch by Filip Pizlo <[email protected]> and Mark Lam <[email protected]> on 2015-10-02
Reviewed by Saam Barati.
Our property type inference has an optimization where the absence of an InferredTypeTable is
taken to mean that all properties are TOP. This is great because most Structures come into
existence through reflective stores, and we don't want to waste time inferring types for
those.
But our code was not obeying this rule properly. If we were doing a transition, we would
assume that this meant that we were creating a new structure, and so we would give it an
InferredTypeTable if we were doing a non-reflective store (i.e. context = PutById). But that
same structure could already have been in use prior to us giving it an InferredTypeTable. At
that point bad things will start to happen because the objects created before we created the
table, and the inline caches compiled before then, will have types that disagree with the new
objects and inline caches despite them sharing the same structure and property names.
* runtime/JSObject.h:
(JSC::JSObject::putDirectInternal):
(JSC::JSObject::putDirectWithoutTransition):
* runtime/Structure.h:
* tests/stress/add-inferred-type-table-to-existing-structure.js: Added.
(foo):
(bar):
(baz):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (190528 => 190529)
--- trunk/Source/_javascript_Core/ChangeLog 2015-10-02 22:33:00 UTC (rev 190528)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-10-02 23:09:33 UTC (rev 190529)
@@ -1,3 +1,33 @@
+2015-10-02 Filip Pizlo <[email protected]> and Mark Lam <[email protected]>
+
+ We should not add InferredTypeTables to old Structures
+ https://bugs.webkit.org/show_bug.cgi?id=149767
+ rdar://problem/22825526
+
+ Reviewed by Saam Barati.
+
+ Our property type inference has an optimization where the absence of an InferredTypeTable is
+ taken to mean that all properties are TOP. This is great because most Structures come into
+ existence through reflective stores, and we don't want to waste time inferring types for
+ those.
+
+ But our code was not obeying this rule properly. If we were doing a transition, we would
+ assume that this meant that we were creating a new structure, and so we would give it an
+ InferredTypeTable if we were doing a non-reflective store (i.e. context = PutById). But that
+ same structure could already have been in use prior to us giving it an InferredTypeTable. At
+ that point bad things will start to happen because the objects created before we created the
+ table, and the inline caches compiled before then, will have types that disagree with the new
+ objects and inline caches despite them sharing the same structure and property names.
+
+ * runtime/JSObject.h:
+ (JSC::JSObject::putDirectInternal):
+ (JSC::JSObject::putDirectWithoutTransition):
+ * runtime/Structure.h:
+ * tests/stress/add-inferred-type-table-to-existing-structure.js: Added.
+ (foo):
+ (bar):
+ (baz):
+
2015-10-02 Joseph Pecoraro <[email protected]>
Unreviewed, rolling out r190520, some tests assert / crash.
Modified: trunk/Source/_javascript_Core/runtime/JSObject.h (190528 => 190529)
--- trunk/Source/_javascript_Core/runtime/JSObject.h 2015-10-02 22:33:00 UTC (rev 190528)
+++ trunk/Source/_javascript_Core/runtime/JSObject.h 2015-10-02 23:09:33 UTC (rev 190529)
@@ -1232,7 +1232,7 @@
Structure* newStructure = Structure::addPropertyTransitionToExistingStructure(
structure, propertyName, attributes, offset);
if (newStructure) {
- newStructure->willStoreValueForTransition(
+ newStructure->willStoreValueForExistingTransition(
vm, propertyName, value, slot.context() == PutPropertySlot::PutById);
DeferGC deferGC(vm.heap);
@@ -1283,7 +1283,7 @@
newStructure = Structure::addPropertyTransition(
vm, structure, propertyName, attributes, offset, slot.context(), &deferredWatchpointFire);
- newStructure->willStoreValueForTransition(
+ newStructure->willStoreValueForNewTransition(
vm, propertyName, value, slot.context() == PutPropertySlot::PutById);
validateOffset(offset);
@@ -1354,7 +1354,7 @@
Structure* structure = this->structure();
PropertyOffset offset = structure->addPropertyWithoutTransition(vm, propertyName, attributes);
bool shouldOptimize = false;
- structure->willStoreValueForTransition(vm, propertyName, value, shouldOptimize);
+ structure->willStoreValueForNewTransition(vm, propertyName, value, shouldOptimize);
setStructureAndButterfly(vm, structure, newButterfly);
putDirect(vm, offset, value);
}
Modified: trunk/Source/_javascript_Core/runtime/Structure.h (190528 => 190529)
--- trunk/Source/_javascript_Core/runtime/Structure.h 2015-10-02 22:33:00 UTC (rev 190528)
+++ trunk/Source/_javascript_Core/runtime/Structure.h 2015-10-02 23:09:33 UTC (rev 190529)
@@ -505,13 +505,28 @@
return InferredType::Top;
}
- ALWAYS_INLINE void willStoreValueForTransition(
+ // Call this when we know that this is a brand new property. Note that it's not enough for the
+ // property to be brand new to some object. It has to be brand new to the Structure.
+ ALWAYS_INLINE void willStoreValueForNewTransition(
VM& vm, PropertyName propertyName, JSValue value, bool shouldOptimize)
{
if (hasBeenDictionary() || (!shouldOptimize && !m_inferredTypeTable))
return;
willStoreValueSlow(vm, propertyName, value, shouldOptimize, InferredTypeTable::NewProperty);
}
+
+ // Call this when we know that this is a new property for the object, but not new for the
+ // structure. Therefore, under the InferredTypeTable's rules, absence of the property from the
+ // table means Top rather than Bottom.
+ ALWAYS_INLINE void willStoreValueForExistingTransition(
+ VM& vm, PropertyName propertyName, JSValue value, bool shouldOptimize)
+ {
+ if (hasBeenDictionary() || !m_inferredTypeTable)
+ return;
+ willStoreValueSlow(vm, propertyName, value, shouldOptimize, InferredTypeTable::NewProperty);
+ }
+
+ // Call this when we know that the inferred type table exists and has an entry for this property.
ALWAYS_INLINE void willStoreValueForReplace(
VM& vm, PropertyName propertyName, JSValue value, bool shouldOptimize)
{
Added: trunk/Source/_javascript_Core/tests/stress/add-inferred-type-table-to-existing-structure.js (0 => 190529)
--- trunk/Source/_javascript_Core/tests/stress/add-inferred-type-table-to-existing-structure.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/add-inferred-type-table-to-existing-structure.js 2015-10-02 23:09:33 UTC (rev 190529)
@@ -0,0 +1,31 @@
+function foo(o) {
+ return o.f + 1;
+}
+
+function bar(o, i, v) {
+ o[i] = v;
+}
+
+function baz() {
+ return {};
+}
+
+noInline(foo);
+noInline(bar);
+noInline(baz);
+
+var o0 = baz();
+bar(o0, "f", "hello");
+
+for (var i = 0; i < 10000; ++i) {
+ var o = baz();
+ o.f = 42;
+ var result = foo(o);
+ if (result != 43)
+ throw "Error: bad result in loop: " + result;
+}
+
+var result = foo(o0);
+if (result != "hello1")
+ throw "Error: bad result at end: " + result;
+
