Bug 67494 - Use bump allocator for initial property storage
Summary: Use bump allocator for initial property storage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on: 67595
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-02 09:51 PDT by Oliver Hunt
Modified: 2011-09-08 15:51 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.33 KB, patch)
2011-09-02 10:08 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (16.17 KB, patch)
2011-09-02 12:49 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (17.54 KB, patch)
2011-09-08 10:00 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (17.84 KB, patch)
2011-09-08 10:11 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (20.99 KB, patch)
2011-09-08 12:43 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (29.86 KB, patch)
2011-09-08 15:08 PDT, Oliver Hunt
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-09-02 09:51:31 PDT
Use bump allocator for initial property storage
Comment 1 Oliver Hunt 2011-09-02 10:08:33 PDT
Created attachment 106148 [details]
Patch
Comment 2 WebKit Review Bot 2011-09-02 11:37:02 PDT
Comment on attachment 106148 [details]
Patch

Attachment 106148 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9581774
Comment 3 Gavin Barraclough 2011-09-02 12:33:00 PDT
Does not pass EWS?
r+, assuming it works!
Comment 4 Oliver Hunt 2011-09-02 12:49:54 PDT
Created attachment 106177 [details]
Patch
Comment 5 Darin Adler 2011-09-02 12:53:35 PDT
Comment on attachment 106177 [details]
Patch

Should fix a spelling error: The word “nursery” has an e in it, not an a.
Comment 6 Oliver Hunt 2011-09-02 13:43:02 PDT
Committed r94445: <http://trac.webkit.org/changeset/94445>
Comment 7 Sam Weinig 2011-09-02 21:26:01 PDT
Comment on attachment 106177 [details]
Patch

Gavin Barraclough reviewed this.  (Oliver doesn't seem to know how to close bugs fully...Oliver).
Comment 8 Alexey Proskuryakov 2011-09-03 22:59:53 PDT
This broke MobileMe Gallery and Yahoo! Mail (bug 67565, bug 67560).
Comment 9 Alexey Proskuryakov 2011-09-03 23:08:57 PDT
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.
Comment 10 Alexey Proskuryakov 2011-09-04 21:26:23 PDT
Likely Twitter, too (bug 67578).

Is anyone available to fix or rollout this?
Comment 11 Csaba Osztrogonác 2011-09-05 03:55:05 PDT
(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.)
Comment 12 Csaba Osztrogonác 2011-09-05 03:55:29 PDT
Reopen, because it was rolled out.
Comment 13 Oliver Hunt 2011-09-08 10:00:40 PDT
Created attachment 106753 [details]
Patch
Comment 14 Oliver Hunt 2011-09-08 10:11:34 PDT
Created attachment 106754 [details]
Patch
Comment 15 Geoffrey Garen 2011-09-08 10:25:12 PDT
"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.
Comment 16 Oliver Hunt 2011-09-08 10:35:20 PDT
(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 17 Gavin Barraclough 2011-09-08 10:40:02 PDT
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 18 Gavin Barraclough 2011-09-08 10:41:07 PDT
Comment on attachment 106754 [details]
Patch

Hang on.  I didn 't mean to r+ this.  Back to r? :-)
Comment 19 Oliver Hunt 2011-09-08 10:55:37 PDT
(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.
Comment 20 Gavin Barraclough 2011-09-08 11:09:00 PDT
(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 21 Gavin Barraclough 2011-09-08 11:21:47 PDT
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 22 Gavin Barraclough 2011-09-08 11:23:27 PDT
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.
Comment 23 Oliver Hunt 2011-09-08 12:43:03 PDT
Created attachment 106773 [details]
Patch
Comment 24 Geoffrey Garen 2011-09-08 13:50:58 PDT
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.
Comment 25 Oliver Hunt 2011-09-08 15:08:29 PDT
Created attachment 106798 [details]
Patch
Comment 26 Geoffrey Garen 2011-09-08 15:21:35 PDT
Comment on attachment 106798 [details]
Patch

r=me
Comment 27 Oliver Hunt 2011-09-08 15:51:41 PDT
Committed r94814: <http://trac.webkit.org/changeset/94814>