Move ArrayBuffer view list into per compartment tables

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

unspecified
mozilla35
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8482496 [details] [diff] [review]
patch.txt

The view list stored on ArrayBufferObjects requires slots on the buffer objects themselves and all views.  For bug 1058340 I'd like to remove these slots, but would like to still know all the views for each buffer.  The attached patch removes the threaded view list and a lot of associated complexity, and adds per-compartment tables mapping buffers to their views.  This is a more efficient use of memory when most array buffers do not need to be explicitly created, such as when lots of small typed arrays or (with bug 1058340) lots of typed objects are created.  After bug 1058340 these tables will also keep track of any subobjects created for a given typed object.

For array buffers, table entries are only added for buffers with multiple views, i.e. those which are currently added to the gcLiveArrayBuffers lists during GC anyways.  This avoids some performance pitfalls that could arise if the tables also reflected buffers with single views; the main difference during tracing is that the first view is always held strongly by the buffer.  Instead of accumulating the live buffers during tracing though, the tables in this bug are always complete, which simplifies things a lot.
(Assignee)

Updated

3 years ago
Attachment #8482496 - Attachment filename: patch.txt → patch
Attachment #8482496 - Attachment is patch: true
Attachment #8482496 - Flags: review?(wmccloskey)
Attachment #8482496 - Flags: review?(sphink)
(Assignee)

Updated

3 years ago
Blocks: 1061741
Whiteboard: [MemShrink]
Comment on attachment 8482496 [details] [diff] [review]
patch.txt

Review of attachment 8482496 [details] [diff] [review]:
-----------------------------------------------------------------

Darn it, why didn't I think of doing it this way? I like it way better than what I did.

::: js/src/vm/ArrayBufferObject.cpp
@@ +782,5 @@
> +
> +bool
> +InnerViewTable::addView(JSContext *cx, JSObject *obj, ArrayBufferViewObject *view)
> +{
> +    // ArrayBufferObject entries are only added when there are multople views.

*multiple

@@ +784,5 @@
> +InnerViewTable::addView(JSContext *cx, JSObject *obj, ArrayBufferViewObject *view)
> +{
> +    // ArrayBufferObject entries are only added when there are multople views.
> +    JS_ASSERT_IF(obj->is<ArrayBufferObject>(),
> +                 obj->as<ArrayBufferObject>().firstView());

Seems like this ought to just be

  JS_ASSERT(obj->as<ArrayBufferObject>().firstView()));

for now, so that when someone comes along and adds a new type of object with views, this will fail the type check so they'll know to add an assert specific to their new type (and change this assert to what you have now.) Or maybe add a redundant assert obj->is<ArrayBufferObject>() to make it a little more clear?

::: js/src/vm/ArrayBufferObject.h
@@ +93,5 @@
>  
>      enum BufferKind {
>          PLAIN_BUFFER        =   0, // malloced or inline data
>          ASMJS_BUFFER        = 0x1,
> +        MAPPED_BUFFER       = 0x2,

Whoa, where'd SHARED_BUFFER go? I just updated, and it's still there. (It should be ok, though. SharedArrayBuffers are not neuterable.)

@@ +205,5 @@
>                                         JS::ClassInfo *info);
>  
> +    // ArrayBufferObjects (strongly) store the first view added to them, to
> +    // optimize for the common case. Later views are (weakly) stored in the
> +    // compartment's InnerViewTable below.

Need to explain in this comment why it's an optimization. Though to be honest, I just reread bug 789295, and I'm not 100% convinced that it is. All I know that (1) BananaBread janks horribly if you prevent views from being background finalized, and (2) if you include buffers with a single view in the table, then you'll end up doing the same amount of mainthread sweep work as you would have with a foreground finalizer. But if you add in that (3) these first-view strong edges add an equivalent amount of work during mark time, which is also mainthread, then this optimization is depending on time(1) == time(2) > time(3). And I don't know why any of those relations are necessarily true. I guess marking is incremental. And we have empirical evidence that jank(1) > jank(3).

I must be missing something.

@@ +448,5 @@
>  template<> inline bool TypeIsUnsigned<uint32_t>() { return true; }
>  
> +// Per-compartment table which manages the relationship between array buffers
> +// and typed objects which own their own storage and the views which use the
> +// same storage for their data.

I'm working hard to suppress the urge to pedantically insist that all of your "which"es should be "that"s. (I still remember Alex Aiken complaining about that on my thesis, and me not having any clue what he was going on about. Now I pay attention to restrictive clauses.)

But it really doesn't matter.

@@ +482,5 @@
> +
> +    // Get any views for this object. Note that no barriers are in place in
> +    // this vector, so any storing of these views on the heap must be
> +    // accompanied by explicit read barrier invocations.
> +    ViewVector *maybeViews(JSObject *obj);

Hmm. That suggests maybeViewsUnbarriered.

@@ +498,5 @@
> +    void sweep(JSRuntime *rt);
> +    void sweepAfterMinorGC(JSRuntime *rt);
> +
> +    bool needsSweepAfterMinorGC() {
> +        return !nurseryKeys.empty();

should this be

  return !nurseryKeys.empty() || !nurseryKeysValid

? Or perhaps

  JS_ASSERT_IF(nurseryKeys.empty(), nurseryKeysValid);

?
Attachment #8482496 - Flags: review?(sphink) → review+
(Assignee)

Comment 2

3 years ago
(In reply to Steve Fink [:sfink] from comment #1)
> ::: js/src/vm/ArrayBufferObject.h
> @@ +93,5 @@
> >  
> >      enum BufferKind {
> >          PLAIN_BUFFER        =   0, // malloced or inline data
> >          ASMJS_BUFFER        = 0x1,
> > +        MAPPED_BUFFER       = 0x2,
> 
> Whoa, where'd SHARED_BUFFER go? I just updated, and it's still there. (It
> should be ok, though. SharedArrayBuffers are not neuterable.)

Ah, sorry, I forgot to mention that this patch (as well as bug 1061741) goes on top of the patches in bug 1054882, and won't land until after that bug does.
Depends on: 1054882
(Assignee)

Comment 3

3 years ago
(In reply to Steve Fink [:sfink] from comment #1)
> @@ +205,5 @@
> >                                         JS::ClassInfo *info);
> >  
> > +    // ArrayBufferObjects (strongly) store the first view added to them, to
> > +    // optimize for the common case. Later views are (weakly) stored in the
> > +    // compartment's InnerViewTable below.
> 
> Need to explain in this comment why it's an optimization. Though to be
> honest, I just reread bug 789295, and I'm not 100% convinced that it is. All
> I know that (1) BananaBread janks horribly if you prevent views from being
> background finalized, and (2) if you include buffers with a single view in
> the table, then you'll end up doing the same amount of mainthread sweep work
> as you would have with a foreground finalizer. But if you add in that (3)
> these first-view strong edges add an equivalent amount of work during mark
> time, which is also mainthread, then this optimization is depending on
> time(1) == time(2) > time(3). And I don't know why any of those relations
> are necessarily true. I guess marking is incremental. And we have empirical
> evidence that jank(1) > jank(3).

The main thing here is that (3) is incrementalized, while (2) is not.  Initially I wrote a patch that didn't have this first view optimization, and while measuring GC time with a few million single view buffers I was seeing 200ms+ pauses to non-incrementally sweep the views table.  With table entries only added for buffers with multiple views, we shouldn't see pause times any worse with this patch than we currently do sweeping the gcLiveArrayBuffers list.
Oh, awesome, thanks!

So then the comment can just add that marking takes time proportional to the number of live views, which can get large and therefore needs to be incremental (incrementalizable?).
Comment on attachment 8482496 [details] [diff] [review]
patch.txt

This sounds really cool. I'm sorry I don't have time to look at it right now. Steve's review is sufficient though.
Attachment #8482496 - Flags: review?(wmccloskey)
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3da4ca374cf
https://hg.mozilla.org/mozilla-central/rev/e3da4ca374cf
Assignee: nobody → bhackett1024
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(Assignee)

Updated

3 years ago
Depends on: 1069680
Depends on: 1082141
You need to log in before you can comment on or make changes to this bug.