Title: [292594] trunk
Revision
292594
Author
ysuz...@apple.com
Date
2022-04-07 23:58:47 -0700 (Thu, 07 Apr 2022)

Log Message

[JSC] Fire structure transition watchpoint in Structure::finishCreation instead of Structure constructor
https://bugs.webkit.org/show_bug.cgi?id=238980

Reviewed by Saam Barati.

JSTests:

* stress/heap-allocation-in-did-structure-transition-watchpoint.js: Added.
(__isPropertyOfType):
(__getProperties):
(__getObjects):
(__getRandomObject):
(__getRandomProperty):
(__callGC):
(get var):

Source/_javascript_Core:

After https://github.com/WebKit/WebKit/commit/dc3a347474a183891f8e07966dc09e684d7a1d13 change,
we start using Structure::get in the main thread. However one of the difference between Structure::get and
Structure::getConcurrently is that it can allocate GC memory: PropertyTable can be materialized.

Structure constructor was firing structure transition watchpoint. And some of watchpoints were using
Structure::getConcurrently. That's fine before, but now, it becomes Structure::get. It is not OK since
we cannot allocate GC memory inside constructor of GC managed objects.

This patch split didTransitionFromThisStructure into didTransitionFromThisStructureWithoutFiringWatchpoint and
fireStructureTransitionWatchpoint. And firing watchpoints in Structure::finishCreation instead of Structure
constructor so that we can allocate GC memory while firing watchpoints.

* runtime/BrandedStructure.cpp:
(JSC::BrandedStructure::BrandedStructure):
(JSC::BrandedStructure::create):
* runtime/BrandedStructure.h:
* runtime/Structure.cpp:
(JSC::Structure::Structure):
(JSC::Structure::didTransitionFromThisStructureWithoutFiringWatchpoint const):
(JSC::Structure::fireStructureTransitionWatchpoint const):
(JSC::Structure::didTransitionFromThisStructure const):
* runtime/Structure.h:
(JSC::Structure::finishCreation):
* runtime/StructureInlines.h:
(JSC::Structure::create):

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChangeLog (292593 => 292594)


--- trunk/JSTests/ChangeLog	2022-04-08 05:40:06 UTC (rev 292593)
+++ trunk/JSTests/ChangeLog	2022-04-08 06:58:47 UTC (rev 292594)
@@ -1,3 +1,19 @@
+2022-04-07  Yusuke Suzuki  <ysuz...@apple.com>
+
+        [JSC] Fire structure transition watchpoint in Structure::finishCreation instead of Structure constructor
+        https://bugs.webkit.org/show_bug.cgi?id=238980
+
+        Reviewed by Saam Barati.
+
+        * stress/heap-allocation-in-did-structure-transition-watchpoint.js: Added.
+        (__isPropertyOfType):
+        (__getProperties):
+        (__getObjects):
+        (__getRandomObject):
+        (__getRandomProperty):
+        (__callGC):
+        (get var):
+
 2022-04-07  Geza Lore  <gl...@igalia.com>
 
         [JSC][ARMv7] Support proper near calls and JUMP_ISLANDS

Added: trunk/JSTests/stress/heap-allocation-in-did-structure-transition-watchpoint.js (0 => 292594)


--- trunk/JSTests/stress/heap-allocation-in-did-structure-transition-watchpoint.js	                        (rev 0)
+++ trunk/JSTests/stress/heap-allocation-in-did-structure-transition-watchpoint.js	2022-04-08 06:58:47 UTC (rev 292594)
@@ -0,0 +1,33 @@
+function __isPropertyOfType() {
+}
+function __getProperties(obj) {
+  let properties = [];
+  for (let name of Object.getOwnPropertyNames(obj)) {
+ properties.push();
+  }
+}
+function* __getObjects() {
+  let obj_names = __getProperties( 'object');
+}
+function __getRandomObject() {
+  for (let obj of __getObjects()) {
+  }
+}
+function __getRandomProperty() {
+}
+(function () {
+  __callGC = function () {
+      gc();
+  };
+})();
+  Array.prototype[2] = undefined;
+__getRandomObject(), {
+      get: function () {
+      }
+    };
+    var __v_18 = Function();
+  delete __v_18[__getRandomProperty()], __callGC();
+  for (var __v_26 = 0; __v_26 < 10; ++__v_26) {
+      Object.defineProperty(Array.prototype, __v_26, {
+      });
+  }
\ No newline at end of file

Modified: trunk/Source/_javascript_Core/ChangeLog (292593 => 292594)


--- trunk/Source/_javascript_Core/ChangeLog	2022-04-08 05:40:06 UTC (rev 292593)
+++ trunk/Source/_javascript_Core/ChangeLog	2022-04-08 06:58:47 UTC (rev 292594)
@@ -1,3 +1,36 @@
+2022-04-07  Yusuke Suzuki  <ysuz...@apple.com>
+
+        [JSC] Fire structure transition watchpoint in Structure::finishCreation instead of Structure constructor
+        https://bugs.webkit.org/show_bug.cgi?id=238980
+
+        Reviewed by Saam Barati.
+
+        After https://github.com/WebKit/WebKit/commit/dc3a347474a183891f8e07966dc09e684d7a1d13 change,
+        we start using Structure::get in the main thread. However one of the difference between Structure::get and
+        Structure::getConcurrently is that it can allocate GC memory: PropertyTable can be materialized.
+
+        Structure constructor was firing structure transition watchpoint. And some of watchpoints were using
+        Structure::getConcurrently. That's fine before, but now, it becomes Structure::get. It is not OK since
+        we cannot allocate GC memory inside constructor of GC managed objects.
+
+        This patch split didTransitionFromThisStructure into didTransitionFromThisStructureWithoutFiringWatchpoint and
+        fireStructureTransitionWatchpoint. And firing watchpoints in Structure::finishCreation instead of Structure
+        constructor so that we can allocate GC memory while firing watchpoints.
+
+        * runtime/BrandedStructure.cpp:
+        (JSC::BrandedStructure::BrandedStructure):
+        (JSC::BrandedStructure::create):
+        * runtime/BrandedStructure.h:
+        * runtime/Structure.cpp:
+        (JSC::Structure::Structure):
+        (JSC::Structure::didTransitionFromThisStructureWithoutFiringWatchpoint const):
+        (JSC::Structure::fireStructureTransitionWatchpoint const):
+        (JSC::Structure::didTransitionFromThisStructure const):
+        * runtime/Structure.h:
+        (JSC::Structure::finishCreation):
+        * runtime/StructureInlines.h:
+        (JSC::Structure::create):
+
 2022-04-07  Elliott Williams  <e...@apple.com>
 
         [XCBuild] Enable dependency validation by default

Modified: trunk/Source/_javascript_Core/runtime/BrandedStructure.cpp (292593 => 292594)


--- trunk/Source/_javascript_Core/runtime/BrandedStructure.cpp	2022-04-08 05:40:06 UTC (rev 292593)
+++ trunk/Source/_javascript_Core/runtime/BrandedStructure.cpp	2022-04-08 06:58:47 UTC (rev 292594)
@@ -31,8 +31,8 @@
 
 namespace JSC {
 
-BrandedStructure::BrandedStructure(VM& vm, Structure* previous, UniquedStringImpl* brandUid, DeferredStructureTransitionWatchpointFire* deferred)
-    : Structure(vm, previous, deferred)
+BrandedStructure::BrandedStructure(VM& vm, Structure* previous, UniquedStringImpl* brandUid)
+    : Structure(vm, previous)
     , m_brand(brandUid)
 {
     if (previous->isBrandedStructure())
@@ -40,8 +40,8 @@
     this->setIsBrandedStructure(true);
 }
 
-BrandedStructure::BrandedStructure(VM& vm, BrandedStructure* previous, DeferredStructureTransitionWatchpointFire* deferred)
-    : Structure(vm, previous, deferred)
+BrandedStructure::BrandedStructure(VM& vm, BrandedStructure* previous)
+    : Structure(vm, previous)
     , m_brand(previous->m_brand)
     , m_parentBrand(vm, this, previous->m_parentBrand.get(), WriteBarrier<BrandedStructure>::MayBeNull)
 {
@@ -51,8 +51,8 @@
 Structure* BrandedStructure::create(VM& vm, Structure* previous, UniquedStringImpl* brandUid, DeferredStructureTransitionWatchpointFire* deferred)
 {
     ASSERT(vm.structureStructure);
-    BrandedStructure* newStructure = new (NotNull, allocateCell<BrandedStructure>(vm)) BrandedStructure(vm, previous, brandUid, deferred);
-    newStructure->finishCreation(vm, previous);
+    BrandedStructure* newStructure = new (NotNull, allocateCell<BrandedStructure>(vm)) BrandedStructure(vm, previous, brandUid);
+    newStructure->finishCreation(vm, previous, deferred);
     ASSERT(newStructure->type() == StructureType);
     return newStructure;
 }

Modified: trunk/Source/_javascript_Core/runtime/BrandedStructure.h (292593 => 292594)


--- trunk/Source/_javascript_Core/runtime/BrandedStructure.h	2022-04-08 05:40:06 UTC (rev 292593)
+++ trunk/Source/_javascript_Core/runtime/BrandedStructure.h	2022-04-08 06:58:47 UTC (rev 292594)
@@ -68,8 +68,8 @@
     }
 
 private:
-    BrandedStructure(VM&, Structure*, UniquedStringImpl* brand, DeferredStructureTransitionWatchpointFire*);
-    BrandedStructure(VM&, BrandedStructure*, DeferredStructureTransitionWatchpointFire*);
+    BrandedStructure(VM&, Structure*, UniquedStringImpl* brand);
+    BrandedStructure(VM&, BrandedStructure*);
 
     static Structure* create(VM&, Structure*, UniquedStringImpl* brand, DeferredStructureTransitionWatchpointFire* = nullptr);
 

Modified: trunk/Source/_javascript_Core/runtime/Structure.cpp (292593 => 292594)


--- trunk/Source/_javascript_Core/runtime/Structure.cpp	2022-04-08 05:40:06 UTC (rev 292593)
+++ trunk/Source/_javascript_Core/runtime/Structure.cpp	2022-04-08 06:58:47 UTC (rev 292594)
@@ -272,7 +272,7 @@
     ASSERT(!this->typeInfo().overridesGetCallData() || m_classInfo->methodTable.getCallData != &JSCell::getCallData);
 }
 
-Structure::Structure(VM& vm, Structure* previous, DeferredStructureTransitionWatchpointFire* deferred)
+Structure::Structure(VM& vm, Structure* previous)
     : JSCell(vm, vm.structureStructure.get())
     , m_inlineCapacity(previous->m_inlineCapacity)
     , m_bitField(0)
@@ -307,7 +307,9 @@
     ASSERT(!previous->typeInfo().structureIsImmortal());
     setPreviousID(vm, previous);
 
-    previous->didTransitionFromThisStructure(deferred);
+    // Do not fire watchpoint inside Structure constructor since watchpoint can involve further heap allocations.
+    // We fire watchpoint separately in Structure::finishCreation.
+    previous->didTransitionFromThisStructureWithoutFiringWatchpoint();
     
     // Copy this bit now, in case previous was being watched.
     setTransitionWatchpointIsLikelyToBeFired(previous->transitionWatchpointIsLikelyToBeFired());
@@ -1244,7 +1246,7 @@
     out.print("Structure transition from ", *m_structure);
 }
 
-void Structure::didTransitionFromThisStructure(DeferredStructureTransitionWatchpointFire* deferred) const
+void Structure::didTransitionFromThisStructureWithoutFiringWatchpoint() const
 {
     // If the structure is being watched, and this is the kind of structure that the DFG would
     // like to watch, then make sure to note for all future versions of this structure that it's
@@ -1251,7 +1253,10 @@
     // unwise to watch it.
     if (m_transitionWatchpointSet.isBeingWatched())
         const_cast<Structure*>(this)->setTransitionWatchpointIsLikelyToBeFired(true);
+}
 
+void Structure::fireStructureTransitionWatchpoint(DeferredStructureTransitionWatchpointFire* deferred) const
+{
     if (deferred) {
         ASSERT(deferred->structure() == this);
         m_transitionWatchpointSet.fireAll(vm(), deferred);
@@ -1259,6 +1264,12 @@
         m_transitionWatchpointSet.fireAll(vm(), StructureFireDetail(this));
 }
 
+void Structure::didTransitionFromThisStructure(DeferredStructureTransitionWatchpointFire* deferred) const
+{
+    didTransitionFromThisStructureWithoutFiringWatchpoint();
+    fireStructureTransitionWatchpoint(deferred);
+}
+
 template<typename Visitor>
 void Structure::visitChildrenImpl(JSCell* cell, Visitor& visitor)
 {

Modified: trunk/Source/_javascript_Core/runtime/Structure.h (292593 => 292594)


--- trunk/Source/_javascript_Core/runtime/Structure.h	2022-04-08 05:40:06 UTC (rev 292593)
+++ trunk/Source/_javascript_Core/runtime/Structure.h	2022-04-08 06:58:47 UTC (rev 292594)
@@ -136,7 +136,7 @@
     JS_EXPORT_PRIVATE static bool isValidPrototype(JSValue);
 
 protected:
-    void finishCreation(VM& vm, const Structure* previous)
+    void finishCreation(VM& vm, const Structure* previous, DeferredStructureTransitionWatchpointFire* deferred)
     {
         this->finishCreation(vm);
         if (previous->hasRareData()) {
@@ -146,6 +146,7 @@
                 rareData()->setSharedPolyProtoWatchpoint(previousRareData->copySharedPolyProtoWatchpoint());
             }
         }
+        previous->fireStructureTransitionWatchpoint(deferred);
     }
 
 private:
@@ -674,6 +675,10 @@
         m_transitionWatchpointSet.add(watchpoint);
     }
     
+    void didTransitionFromThisStructureWithoutFiringWatchpoint() const;
+    void fireStructureTransitionWatchpoint(DeferredStructureTransitionWatchpointFire*) const;
+
+    // This function does the both didTransitionFromThisStructureWithoutFiringWatchpoint and fireStructureTransitionWatchpoint.
     void didTransitionFromThisStructure(DeferredStructureTransitionWatchpointFire* = nullptr) const;
     
     InlineWatchpointSet& transitionWatchpointSet() const
@@ -764,7 +769,7 @@
     static_assert(s_bitWidthOfTransitionKind <= sizeof(TransitionKind) * 8);
 
 protected:
-    Structure(VM&, Structure*, DeferredStructureTransitionWatchpointFire*);
+    Structure(VM&, Structure*);
 
 private:
     friend class LLIntOffsetsExtractor;

Modified: trunk/Source/_javascript_Core/runtime/StructureInlines.h (292593 => 292594)


--- trunk/Source/_javascript_Core/runtime/StructureInlines.h	2022-04-08 05:40:06 UTC (rev 292593)
+++ trunk/Source/_javascript_Core/runtime/StructureInlines.h	2022-04-08 06:58:47 UTC (rev 292594)
@@ -79,11 +79,10 @@
     ASSERT(vm.structureStructure);
     Structure* newStructure;
     if (previous->isBrandedStructure())
-        newStructure = new (NotNull, allocateCell<BrandedStructure>(vm)) BrandedStructure(vm, jsCast<BrandedStructure*>(previous), deferred);
+        newStructure = new (NotNull, allocateCell<BrandedStructure>(vm)) BrandedStructure(vm, jsCast<BrandedStructure*>(previous));
     else
-        newStructure = new (NotNull, allocateCell<Structure>(vm)) Structure(vm, previous, deferred);
-
-    newStructure->finishCreation(vm, previous);
+        newStructure = new (NotNull, allocateCell<Structure>(vm)) Structure(vm, previous);
+    newStructure->finishCreation(vm, previous, deferred);
     return newStructure;
 }
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to