[webkit-reviews] review denied: [Bug 83839] GC in the middle of JSObject::allocatePropertyStorage can cause badness : [Attachment 137023] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 12 21:52:55 PDT 2012
Geoffrey Garen <ggaren at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 83839: GC in the middle of JSObject::allocatePropertyStorage can cause
badness
https://bugs.webkit.org/show_bug.cgi?id=83839
Attachment 137023: Patch
https://bugs.webkit.org/attachment.cgi?id=137023&action=review
------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=137023&action=review
I think you want an interface like this:
// Add property with structure transition
newStorage = reallocPropertyStorage(m_propertyStorage.get(), ...); // Returns
new storage, does not assign.
newStructure = Structure::addPropertyTransition(...); // Returns new structure,
does not assign.
setPropertyStorage(newStorage, newStructure); // Assigns structure and storage
"atomically".
// Add property without structure transition
newStorage = reallocPropertyStorage(m_propertyStorage.get(), ...); // Returns
new storage, does not assign.
setPropertyStorage(newStorage, m_structure.get()); // Assigns structure and
storage "atomically".
Since storage and structure need to be in sync, we should never set one without
setting the other.
> Source/JavaScriptCore/runtime/JSObject.h:661
> + if (structure()->shouldResizePropertyStorage())
Let's call this "shouldGrowPropertyStorage". "Resize" is imprecise, because we
don't shrink.
> Source/JavaScriptCore/runtime/JSObject.h:662
> + allocatePropertyStorage(globalData,
structure()->propertyStorageCapacity(),
structure()->suggestedPropertyStorageResizeAmount());
Let's call this "growPropertyStorage" or "reallocPropertyStorage" to help
clarify that we're allocating for a second time.
> Source/JavaScriptCore/runtime/JSObject.h:727
> + Structure* structure = Structure::addPropertyTransition(globalData,
this->structure(), propertyName, attributes, specificFunction, offset);
This addPropertyTransition will allocate a new structure, which can cause a GC.
During that GC, our Structure's information about our backing store will be out
of date (specifically, it will underestimate the size / capacity of our backing
store). I'm not sure if that will definitely cause a problem, but it seems not
so good. We never want our information about our backing store to be out of
date.
More information about the webkit-reviews
mailing list