619622 - JM: shrink BasePolyIC by improving the ExecPoolVector representation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch (against 58597:dab87d48d3dd)

Bug 615199 identified the ExecPoolVector in BasePolyIC as a significant cause of memory usage.  Bug 616367 tried to improve this but gave an unclear improvement.  This bug takes a different approach.

I determined that typically 90--95% of BasePolyICs have zero entries in the ExecPoolVector, and over 99% have zero or one.  (By "typically" I tried SS, V8 and a 20-tab cad-comic browser session.)

So I've optimized the storage for those cases:  there's now a single ExecutablePool pointer which can be NULL for zero entries or non-NULL for one entry.  If we get two or more entries, that pointer changes to a pointer to ExecPoolVector (it's in a union) and the bottom bit of that pointer is tagged to indicate we've switched representations.

On 32-bit machines, this reduces the size of a BasePolyIC by 24 bytes, on 64-bit machines it reduces the size by 32 bytes.

For a 20 tab cad-comic run, the amount of memory calloc'd in finishThisUp() dropped by 1.14x, from 30,154,548 bytes to 26,553,256 bytes.  Instruction counts were basically unchanged, this isn't hot code.

I used SystemAllocPolicy() to allocate the ExecPoolVector;  this seems reasonable since that's what's used within the ExecPoolVector itself, but it's different to what's used for the ICs themselves (which are cx->calloc'd).

Luke, I'd appreciate it if you could pay careful attention to the placement new in addPool() and the corresponding deconstruct/free pair in ~BasePolyIC().  Thanks!

Comment 1

[Nicholas Nethercote [inactive]]

This patch looks simpler than the one in bug 616367, too;  the changes are all contained within BasePolyIC.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 498034 [details] [diff] [review]
patch (against 58597:dab87d48d3dd)

Great idea!

Is this the complete patch?  I ask because you've removed reset() and releasePools() without changing the places that will call them.  Furthermore, b/c BasePolyIC is malloc'd and free'd with the whole JITScript, its constructors/destructors won't be called unless you manually do so.  Another comment is that I think you can use plain old 'new' and 'delete' for the vector instead of manual malloc/ctor/dtor/free.

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #2)
> 
> Is this the complete patch?  I ask because you've removed reset() and
> releasePools() without changing the places that will call them.

It was meant to be, I thought there weren't any callers to those functions so I inlined them but now I see that BaseIC has a reset() that BasePolyIC overrides.

> Furthermore,
> b/c BasePolyIC is malloc'd and free'd with the whole JITScript, its
> constructors/destructors won't be called unless you manually do so.  

You sure?  ~JITScript() calls Destroy() on each elements of pics[], which seems to do the trick.  ~BasePolyIC() is definitely being called, if I change it to return immediately I get leak reports from Valgrind.

> Another
> comment is that I think you can use plain old 'new' and 'delete' for the vector
> instead of manual malloc/ctor/dtor/free.

Oh... I avoided that because it doesn't seem to happen anywhere else.  Do they end up taking the same route as SystemAllocPolicy() (aka. js_malloc/js_free)?

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 498034 [details] [diff] [review]
patch (against 58597:dab87d48d3dd)

Good point about releasePools/reset not being called.  Grep was showing me calls on different types.

>+            ExecPoolVector *execPools = new(mem) ExecPoolVector(SystemAllocPolicy()); 
>+            if (!execPools)
>+                return false;
>+            u.taggedExecPools = tag(execPools);
>+            return execPools->append(oldPool) && execPools->append(pool);

This is a very unlikely corner case, but I think you want to get the fallible vector-building out of the way before "committing" the vector to u.taggedExecPools.  That way, if you oom on append(), and the the browser manages to recover (somehow) you don't lose oldPool.  I'm thinking:

  ExecPoolVector *execPools = ...
  if (!execPools)
      return false;
  if (!execPools->append(oldPool) || !execPools->append(pool)) {
      delete execPools;
      return false;
  }
  u.taggedExecPools = tag(execPools);
  return true;

r+ with that and using new/delete as described above.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: patch v2 (against TM 58597:dab87d48d3dd)

The memory management and destructors are gnarly enough here that I'd like another review, please :)

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 498252 [details] [diff] [review]
patch v2 (against TM 58597:dab87d48d3dd)

Sorry for the delay; I got interrupted and this got paged out.

r+. Only nit is that these two-liners could be one-liners:

>+            ExecPoolVector *execPools = multiplePools();
>+            delete execPools;

>+            ExecPoolVector *execPools = multiplePools();
>+            execPools->clear();

>+        ExecPoolVector *execPools = multiplePools();
>+        return execPools->append(pool);

Comment 7

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/7d02185c8366

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/7d02185c8366