616367 - JM: Move BasePolyIC::execPools to JaegerCompartment

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Since these pools are cleared on each garbage collection, we can hoist the vector of pools from BasePolyIC into JaegerCompartment. Trade-off is that we'll waste extra space because js::Vector over-allocates.

Luke says we can tweak js::Vector to help.

Comment 1

[Brian Hackett [Laid off!]]

Crude, but you could maybe fix the overallocation by splitting the pools into multiple smaller allocations, e.g. have a vector of fixed-size calloc'ed arrays.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Even without js::Vector help this seems to have worthwhile savings. Some statistics across 2 GCs for v8, before & after (all savings are relative to all JITScripts allocated before each GC starts, not include code memory):

GC #1:
  13% less memory total (27% of PIC bookkeeping)
  85% increase in overallocation (0.2% of total)
GC #2:
  17% less memory total (27% of PIC bookkeeping)
  22% increase in overallocation (0.7% of total)

Please ignore all the spew stuff in this patch, it's instrumentation earlier in my queue (I'll get it checked in separately).

Comment 3

[Luke Wagner [:luke]]

Attachment: add v.clearAndFree()

v.clear() doesn't free the underlying buffer, so this patch adds v.clearAndFree() which does.  This can be called from gc after iterating through the vector.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 494915 [details] [diff] [review]
add v.clearAndFree()

> 
>     void clear();
>+    void clearAndFree();

Comments on these, please.  One on popBack() would be nice, too :)

r=me with that fixed.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fix

Uses Luke's patch right above, also takes two words out of CallICInfo.

Comment 6

[Julian Seward [:jseward]]

Profiling (TM + the c3 + c5 patches + the ntab patch) vs (TM + the
ntab patch) w/ a complete browser run, loading 30 of the cad-comic
tabs used in bug 615199, I am having great difficulty in showing any
repeatable space gain.

For both with and without the c3 & c5 patches, the maximum heap size
comes in around 320 - 330MB.  But it's noisy, and despite several
profiling runs with efforts taken to make them as identical as
possible, I can't see any consistent gain.  The peak heap numbers
differ randomly by up to about 8MB.  Profiling at the page level
instead of heap doesn't fare any better.

Can you give any formula, based on sizeof(ic::PICInfo), pics.length(),
whatever, in mjit::Compiler::finishThisUp, by which I could estimate
the expected space savings?

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

You should save, per-script:

  sizeof(ExecPoolVector) * pics.length() +
  sizeof(void *) * 2 * callICs.length()  +
  sizeof(ExecPoolVector) * setElems.length()

However, there is a chance that we waste more space because everything was hoisted to one vector (in the compartment), and vectors grow exponentially... that might be what happened.

Comment 8

[Luke Wagner [:luke]]

It would be useful to find out how much memory was in ephemeralICs total and used.  If that was indeed the problem, we can keep avoid adding the same pool to ephemeralICs multiple times.  If each pool is 64K, then the size of ephemeralICs should be quite small, exponential growth or not.

Comment 9

[David Mandelin [:dmandelin]]

Unblocking because Julian measured no repeatable memory improvement.

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to comment #9)
> Unblocking because Julian measured no repeatable memory improvement.

I've filed bug 619622 which is an alternative approach to this problem.  I nominated it as blocking -- just because this patch didn't work doesn't mean we should declare the problem solved...

Comment 11

[Nicholas Nethercote [inactive]]

I'm closing this in favour of bug 619622.