669005 - Large ArrayBuffers aren't counted towards a compartment in about:memory

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

Attachment: Testcase

STR:

 * Open testcase
 * Open about:memory

Expected results:

about:memory shows testcase's compartment using at least 50mb of memory, since testcase allocates and writes to a 50mb array buffer.

Actual results:

about:memory shows testcase compartment using less than 1mb.  In fact, explicit size doesn't increase.  But RSS increases by 50mb, so I presume that memory is actually being allocated for the arraybuffer.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: about:memory before loading testcase

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: about:memory after loading testcase (people.mozilla.org compartment)

Comment 3

[Justin Lebar (not reading bugmail)]

Mozilla/5.0 (X11; Linux x86_64; rv:7.0a1) Gecko/20110701 Firefox/7.0a1

Comment 4

[Nicholas Nethercote [inactive]]

Hmm, weird.  I would have thought that typed array buffers would be covered by the compartment's "object-slots" measurement.

Why do you have this?

  0.00 MB -- heap-used [*]
  0.00 MB -- heap-unused [*]

I only see that if I build with --disable-jemalloc.

Do you need a 'new' operator in front of ArrayBuffer() and Int32Array()?

Comment 5

[Justin Lebar (not reading bugmail)]

(In reply to comment #4)
> Why do you have this?
> 
>   0.00 MB -- heap-used [*]
>   0.00 MB -- heap-unused [*]
> 
> I only see that if I build with --disable-jemalloc.

I was testing with a debug build, because I didn't think it mattered.  I just tested in a release build, and the memory is now in explicit, which is good.  But it's still not in a compartment.
 
> Do you need a 'new' operator in front of ArrayBuffer() and Int32Array()?

It doesn't seem to matter?  I should really learn JavaScript one of these days...  I'll post new results (with 'new' and a release build) in a moment.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Testcase v2 (with operator new)

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: about:memory before loading testcase v2

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: about:memory after loading testcase v2 (people.mozilla.org compartment)

Comment 9

[Andrew McCreight [:mccr8] (PTO until 12-2)]

Could the recently landed typed array improvements (bug 656519 and friends) be a problem?  Though that looks like it only effects small ones.

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> Could the recently landed typed array improvements (bug 656519 and friends)
> be a problem?

Looks like it.  The problem is that an ArrayBuffer JSObject has a slots array, but it doesn't set obj->capacity appropriately in AllocateSlots().  It's not even clear what capacity means for a typed array, since it measure the number of slots, ie. js::Values, but in typed arrays the slots array in bastardized to hold raw bytes and its size may not be a multiple of sizeof(js::Value).

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: draft patch

This patch:

- Sets obj->capacity in an ArrayBuffer, so the object-slots reporter works again.  object-slots may underestimate slightly, however, because the array size might not be a multiple of sizeof(js::Value), which is 8 bytes.

- Uses a temporary so that if the ArrayBuffer slots allocation fails, the old value isn't trashed.  This idiom is used everywhere else that slots are allocated.

Sample about:memory output (for a tweaked version that allocates a 500MB array):

564,801,168 B (100.0%) -- explicit
├──540,138,619 B (95.63%) -- js
│  ├──524,378,156 B (92.84%) -- compartment(file:///home/njn/ta.html)
│  │  ├──524,295,832 B (92.83%) -- object-slots
│  │  ├───────81,920 B (00.01%) -- gc-heap
│  │  │       ├──28,000 B (00.00%) -- objects
│  │  │       ├──26,768 B (00.00%) -- arena-unused
│  │  │       ├──25,344 B (00.00%) -- shapes
│  │  │       ├───1,328 B (00.00%) -- arena-padding
│  │  │       ├─────480 B (00.00%) -- arena-headers
│  │  │       ├───────0 B (00.00%) -- strings
│  │  │       └───────0 B (00.00%) -- xml
│  │  ├──────────404 B (00.00%) -- scripts
│  │  ├────────────0 B (00.00%) -- string-chars
│  │  ├────────────0 B (00.00%) -- mjit-code
│  │  ├────────────0 B (00.00%) -- mjit-data
│  │  ├────────────0 B (00.00%) -- tjit-code
│  │  └────────────0 B (00.00%) -- tjit-data
│  │               ├──0 B (00.00%) -- allocators-main
│  │               └──0 B (00.00%) -- allocators-reserve

I confirmed that this works if you allocate the array directly via Int32Array instead of taking the two-step route via ArrayBuffer.

Nikhil, will setting obj->capacity for an ArrayBuffer have any bad side-effects?
Presumably not, since AllocateSlots leaves it with a non-meaningful value, but I've only run trace-tests so far (they passed).

Also, AllocateSlots doesn't deallocate the old slots if they exist!  Leak!

The other way to fix this is to handle typed array objects specially in CellCallback(), but that doesn't make me very happy.

Comment 12

[Nicholas Nethercote [inactive]]

It would also be nice if jsobj.h had some comments indicating that the slots array can hold raw data for typed arrays.

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 543710 [details] [diff] [review]
draft patch

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

Nicholas,

Setting capacity will have no effects on the ArrayBuffer implementation.
I am guessing that under-reporting memory by 7 bytes at worst is ok with you.
I will fix the de-allocation of reserved slots. Thanks for that!

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Nicholas,

By deallocate old slots, if you mean free()ing the fixed slots, then that is handled automatically by the GC.

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to comment #14)
> 
> By deallocate old slots, if you mean free()ing the fixed slots, then that is
> handled automatically by the GC.

No.  You're replacing one slots array -- which has been manually allocated with malloc() -- with another slots array.  You lose the pointer to the first slots array in the process.

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

no memory is being malloc-ed for slots before that initial call to ArrayBuffer::create, or none should be. Could you please explain?

Comment 17

[Nicholas Nethercote [inactive]]

(In reply to comment #16)
> no memory is being malloc-ed for slots before that initial call to
> ArrayBuffer::create, or none should be. Could you please explain?

Ah, you are right -- I saw in GDB that obj->slots was non-NULL, but it turns out it points to fixedSlots.

Comment 18

[Nicholas Nethercote [inactive]]

(In reply to comment #13)
> 
> Setting capacity will have no effects on the ArrayBuffer implementation.

Turns out that's not true.  I tried setting |capacity| to the number of the bytes in the array, and got lots of these assertions:

  Assertion failure: obj->slotSpan() <= obj->numSlots(), at /home/njn/moz/mi4/js/src/jsgcmark.cpp:623

This doesn't fill me with confidence.

Comment 19

[Nicholas Nethercote [inactive]]

(In reply to comment #18)
> > 
> > Setting capacity will have no effects on the ArrayBuffer implementation.
> 
> Turns out that's not true.

Sorry, ignore that, I got something wrong.

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: patch

Just like the last patch, but with an added assertion and some more comments.

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 544156 [details] [diff] [review]
patch

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

::: js/src/jsobj.h
@@ +305,5 @@
>   * The slots member is a pointer to the slot vector for the object.
>   * This can be either a fixed array allocated immediately after the object,
>   * or a dynamically allocated array.  A dynamic array can be tested for with
> + * hasSlotsArray().  In all cases but one, capacity gives the number of usable
> + * slots;  the exception is for ArrayBuffer where capacity gives the number of

One space after ';' (repeated two lines further, too).

@@ +388,5 @@
>  
>      JSObject    *proto;                     /* object's prototype */
>      JSObject    *parent;                    /* object's parent */
>      void        *privateData;               /* private data */
> +    jsuword     capacity;                   /* number of slots;  for ArrayBuffer the number 

One space after ';'.

::: js/src/jstypedarray.cpp
@@ +143,5 @@
>  {
>      uint32 bytes = size + sizeof(js::Value);
>      if (size > sizeof(js::Value) * ARRAYBUFFER_RESERVED_SLOTS - sizeof(js::Value) ) {
> +        JS_ASSERT(!obj->hasSlotsArray());
> +        js::Value *tmpslots = (js::Value *)cx->calloc_(bytes);

I don't think you need js:: qualification here.  And use reinterpret_cast<>, please.

@@ +150,5 @@
> +        obj->slots = tmpslots;
> +        /*
> +         * Note that |bytes| may not be a multiple of sizeof(js::Value), so
> +         * capacity*sizeof(js::Value) may underestimate the size by up to
> +         * sizeof(js::Value)-1 bytes.

|sizeof(Value) - 1|, note particularly the spaces around the operator.

Comment 22

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c8afe7ab83e7

Comment 23

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/c8afe7ab83e7

Comment 24

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
>  
> > Do you need a 'new' operator in front of ArrayBuffer() and Int32Array()?
> 
> It doesn't seem to matter?  I should really learn JavaScript one of these
> days...  I'll post new results (with 'new' and a release build) in a moment.

Turns out the 'new' is vital.  If you forget it, no object is created, and so 'this' inside the constructor refers to the global object, and you end up setting properties of the global object.