Bug 67494 – Use bump allocator for initial property storage

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Use bump allocator for initial property storage

[Oliver Hunt]

Created attachment 106148 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106148 [details]
Patch

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

[Gavin Barraclough]

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

[Oliver Hunt]

Created attachment 106177 [details]
Patch

[Darin Adler]

Comment on attachment 106177 [details]
Patch

Should fix a spelling error: The word “nursery” has an e in it, not an a.

[Oliver Hunt]

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

[Sam Weinig]

Comment on attachment 106177 [details]
Patch

Gavin Barraclough reviewed this.  (Oliver doesn't seem to know how to close bugs fully...Oliver).

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Likely Twitter, too (bug 67578).

Is anyone available to fix or rollout this?

[Csaba Osztrogonác]

(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.)

[Csaba Osztrogonác]

Reopen, because it was rolled out.

[Oliver Hunt]

Created attachment 106753 [details]
Patch

[Oliver Hunt]

Created attachment 106754 [details]
Patch

[Geoffrey Garen]

"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.

[Oliver Hunt]

(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.

[Gavin Barraclough]

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?

[Gavin Barraclough]

Comment on attachment 106754 [details]
Patch

Hang on.  I didn 't mean to r+ this.  Back to r? :-)

[Oliver Hunt]

(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.

[Gavin Barraclough]

(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.

[Gavin Barraclough]

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

[Gavin Barraclough]

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.

[Oliver Hunt]

Created attachment 106773 [details]
Patch

[Geoffrey Garen]

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.

[Oliver Hunt]

Created attachment 106798 [details]
Patch

[Geoffrey Garen]

Comment on attachment 106798 [details]
Patch

r=me

[Oliver Hunt]

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