Bug 67494

Created attachment 106148 [details] Patch

Comment on attachment 106148 [details] Patch Attachment 106148 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9581774

Does not pass EWS? r+, assuming it works!

Created attachment 106177 [details] Patch

Comment on attachment 106177 [details] Patch Should fix a spelling error: The word “nursery” has an e in it, not an a.

Committed r94445: <http://trac.webkit.org/changeset/94445>

Comment on attachment 106177 [details] Patch Gavin Barraclough reviewed this. (Oliver doesn't seem to know how to close bugs fully...Oliver).

This broke MobileMe Gallery and Yahoo! Mail (bug 67565, bug 67560).

And likely GMail, too (bug 67572). > Committed r94445: <http://trac.webkit.org/changeset/94445> There was also a build fix in r94448, if someone decides to go ahead and roll this change out.

Likely Twitter, too (bug 67578). Is anyone available to fix or rollout this?

(In reply to comment #10) > Likely Twitter, too (bug 67578). > > Is anyone available to fix or rollout this? As you wish, Master. Rollout landed in http://trac.webkit.org/changeset/94522 (Additionally this patch broke hundreds of tests on Qt ARM platform.)

Reopen, because it was rolled out.

Created attachment 106753 [details] Patch

Created attachment 106754 [details] Patch

"NewSpace" is a defunct concept, given the path you're taking. So, I think making this new bump allocator a part of NewSpace is not so good. Would be better to create a new BumpSpace class that's owned by heap, and put this code in there instead. Let's talk this over. > Source/JavaScriptCore/heap/Heap.cpp:693 > + m_newSpace.resetPropertyStorageNursery(); This should be a part of NewSpace::resetAllocator(). Or, even better, BumpSpace::resetAllocator(). > Source/JavaScriptCore/heap/Heap.h:74 > + void addToVisitSet(const JSCell*) { } What's this for? No explanation :(. > Source/JavaScriptCore/heap/Heap.h:97 > + inline void* allocatePropertyStorage(size_t); > + inline bool inPropertyStorageNursery(void*); I think these should be more abstract about what can be stored in them -- allocateInBumpSpace(size_t), inBumpSpace(void*), maybe. > Source/JavaScriptCore/heap/NewSpace.h:49 > + static const size_t PropertyStorageNurserySize = 1024 * 1024 * 4; Please use "KB" instead of 1024. > Source/JavaScriptCore/heap/NewSpace.h:185 > + memset(result, 0, size); Is this needed for correctness? If so, I think it's incorrect on JSValue32_64. If not, please compile it out of release builds. > Source/JavaScriptCore/runtime/JSObject.cpp:612 > + if (usingNursery) > + newPropertyStorage = static_cast<PropertyStorage>(globalData.heap.allocatePropertyStorage(newSize * sizeof(WriteBarrierBase<Unknown>))); > + if (!newPropertyStorage) { > + newPropertyStorage = new WriteBarrierBase<Unknown>[newSize]; > + newAllocationInNursery = false; > + } I think this fact -- that allocation must succeed, and should fall back to some other allocator if necessary -- should move into Heap. Array, etc., will need the same behavior. It's better for Heap to abstract away the details of how the rules of allocation are implemented. > Source/JavaScriptCore/runtime/JSObject.h:904 > + if (Heap::heap(this)->inPropertyStorageNursery(storage)) { > + m_propertyStorage = new WriteBarrierBase<Unknown>[structure()->propertyStorageCapacity()]; > + if (structure()->propertyStorageCapacity() > m_structure->propertyStorageSize()) > + ASSERT(!storage[m_structure->propertyStorageSize()]); > + memcpy(m_propertyStorage, storage, m_structure->propertyStorageSize() * sizeof(WriteBarrierBase<Unknown>)); > + } The SlotVisitor should do the copying, not the object's visit function. Certain kinds of visitors won't want to do this copy. For example, an a Cheney visitor, a verifying visitor, a heap profiling visitor, etc.

(In reply to comment #15) > "NewSpace" is a defunct concept, given the path you're taking. So, I think making this new bump allocator a part of NewSpace is not so good. Would be better to create a new BumpSpace class that's owned by heap, and put this code in there instead. > > Let's talk this over. > > > Source/JavaScriptCore/heap/Heap.cpp:693 > > + m_newSpace.resetPropertyStorageNursery(); > > This should be a part of NewSpace::resetAllocator(). Or, even better, BumpSpace::resetAllocator(). > > > Source/JavaScriptCore/heap/Heap.h:74 > > + void addToVisitSet(const JSCell*) { } > > What's this for? No explanation :(. > > > Source/JavaScriptCore/heap/Heap.h:97 > > + inline void* allocatePropertyStorage(size_t); > > + inline bool inPropertyStorageNursery(void*); > > I think these should be more abstract about what can be stored in them -- allocateInBumpSpace(size_t), inBumpSpace(void*), maybe. That can change once we move out of NewSpace > > > Source/JavaScriptCore/heap/NewSpace.h:49 > > + static const size_t PropertyStorageNurserySize = 1024 * 1024 * 4; > > Please use "KB" instead of 1024. k > > > Source/JavaScriptCore/heap/NewSpace.h:185 > > + memset(result, 0, size); > > Is this needed for correctness? If so, I think it's incorrect on JSValue32_64. If not, please compile it out of release builds. This is needed in order to justify the massively overpowered processor in the users computer :D > > > Source/JavaScriptCore/runtime/JSObject.cpp:612 > > + if (usingNursery) > > + newPropertyStorage = static_cast<PropertyStorage>(globalData.heap.allocatePropertyStorage(newSize * sizeof(WriteBarrierBase<Unknown>))); > > + if (!newPropertyStorage) { > > + newPropertyStorage = new WriteBarrierBase<Unknown>[newSize]; > > + newAllocationInNursery = false; > > + } > > I think this fact -- that allocation must succeed, and should fall back to some other allocator if necessary -- should move into Heap. Array, etc., will need the same behavior. It's better for Heap to abstract away the details of how the rules of allocation are implemented. I agree, when this becomes a more common scenario. Currently it simplifies the patch to do this here. > > > Source/JavaScriptCore/runtime/JSObject.h:904 > > + if (Heap::heap(this)->inPropertyStorageNursery(storage)) { > > + m_propertyStorage = new WriteBarrierBase<Unknown>[structure()->propertyStorageCapacity()]; > > + if (structure()->propertyStorageCapacity() > m_structure->propertyStorageSize()) > > + ASSERT(!storage[m_structure->propertyStorageSize()]); > > + memcpy(m_propertyStorage, storage, m_structure->propertyStorageSize() * sizeof(WriteBarrierBase<Unknown>)); > > + } > > The SlotVisitor should do the copying, not the object's visit function. Certain kinds of visitors won't want to do this copy. For example, an a Cheney visitor, a verifying visitor, a heap profiling visitor, etc. However, doing this currently would complicate the visitor, etc in ways that aren't necessary, and also without other users of the bump allocator i'm not sure what the slot visitor should really be doing.

Comment on attachment 106754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106754&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:-604 > - This function can be in a number of states based on the values of usingNursery, newPropertyStorage, and newAllocationInNursery. I think it would be clear if you deleted the two booleans and added a state enum with named states that better explain what's going on. > Source/JavaScriptCore/runtime/JSObject.cpp:608 > + // Allocation property storage might trigger a GC that will move (typo in comment, nursary). > Source/JavaScriptCore/runtime/JSObject.cpp:614 > + } I think, at this point, either: (1) usingNursery && newPropertyStorage && newAllocationInNursery or: (2) !usingNursery && !newPropertyStorage && newAllocationInNursery > Source/JavaScriptCore/runtime/JSObject.cpp:618 > + } I think, at this point, either: (1) usingNursery && newPropertyStorage && newAllocationInNursery or: (2) !usingNursery && newPropertyStorage && !newAllocationInNursery > Source/JavaScriptCore/runtime/JSObject.cpp:629 > + Based on the observations above, this check is a contradiction? Do we ever need to do this?

Comment on attachment 106754 [details] Patch Hang on. I didn 't mean to r+ this. Back to r? :-)

(In reply to comment #17) > (From update of attachment 106754 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106754&action=review > > > Source/JavaScriptCore/runtime/JSObject.cpp:-604 > > - > > This function can be in a number of states based on the values of usingNursery, newPropertyStorage, and newAllocationInNursery. > I think it would be clear if you deleted the two booleans and added a state enum with named states that better explain what's going on. do you mena enum { NurseryAlloc, OtherAlloc } (or similar) ? > Based on the observations above, this check is a contradiction? Do we ever need to do this? Yes. If you have an object that starts off using a nursery allocation, then the new allocation (through whatever path) does not come from the nursery you will hit this path. The whole point of this path is to catch the conversion from nursery to non-nursery property storage.

(In reply to comment #19) > If you have an object that starts off using a nursery allocation, then the new allocation (through whatever path) does not come from the nursery you will hit this path. Check the code. This does not appear to be true. :-) In the situation you describe, both values will be false.

Comment on attachment 106754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106754&action=review >> Source/JavaScriptCore/runtime/JSObject.cpp:614 >> + } > > I think, at this point, either: > (1) usingNursery && newPropertyStorage && newAllocationInNursery > or: > (2) !usingNursery && !newPropertyStorage && newAllocationInNursery Okay, so there is also: (3) usingNursery && !newPropertyStorage && newAllocationInNursery >> Source/JavaScriptCore/runtime/JSObject.cpp:618 >> + } > > I think, at this point, either: > (1) usingNursery && newPropertyStorage && newAllocationInNursery > or: > (2) !usingNursery && newPropertyStorage && !newAllocationInNursery Okay, so there is also: (3) usingNursery && newPropertyStorage && !newAllocationInNursery

Comment on attachment 106754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106754&action=review > Source/JavaScriptCore/runtime/JSObject.cpp:630 > + if (newAllocationInNursery != usingNursery) Okay, so I think this can only be true of globalData.heap.allocatePropertyStorage returns false, above. You can probably remove newAllocationInNursery, and instead have a simpler needsToAddToVisitSet flag, initialized to false, set of allocatePropertyStorage returns 0.

Created attachment 106773 [details] Patch

Comment on attachment 106773 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106773&action=review Looking a lot better, but still some work to do here. > Source/JavaScriptCore/heap/Heap.cpp:693 > + m_newSpace.resetPropertyStorageNursery(); Please move the bump pointer allocator into a new class in a follow-up patch. > Source/JavaScriptCore/heap/Heap.h:74 > + void addToVisitSet(const JSCell*) { } Oliver said he's going to remove this. > Source/JavaScriptCore/heap/NewSpace.h:49 > + static const size_t PropertyStorageNurserySize = 1024 * 1024 * 4; Please use MB. > Source/JavaScriptCore/runtime/JSObject.cpp:609 > + // If the allocation is too large we don't fit in the nursery, or > + // if allocatePropertyStorage to triggers a GC that promotes us > + // we'll need to allocate storage directly Not so grammatical here. How about: "If allocation failed because it's too big, or it triggered a GC that promoted us to old space, we need to allocate our property storage in old space." Eventually, I think we can remove some of this complexity by moving more responsibility for backing store allocation into the heap, which knows more about these details. > Source/JavaScriptCore/runtime/JSObject.h:75 > + class StorageBarrier { This class should be in its own file. Also, the set function and the constructor need to call Heap::writeBarrier() on the object taking ownership of the storage.

Created attachment 106798 [details] Patch

Comment on attachment 106798 [details] Patch r=me

Committed r94814: <http://trac.webkit.org/changeset/94814>