630447 - JM: shrink JITScript::execPools

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

JITScript has a field, ExecPools, which is a vector.  I've found that it almost always contains zero or one element.  We could do a similar trick to that used in bug 619622, which would reduce sizeof(JITScript) by 20 bytes on 32-bit and 32 bytes on 64-bit.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Fwiw, that looks a lot like nsCheapVoidArray, except without the void* insanity, and with a bit more memory-use in the case of more than one element...

Worth adding to jstl?

Comment 2

[Luke Wagner [:luke]]

Boris, you beat me to it!  I held my tongue the first time we did this trick since there was only use, but now that we have two, sounds like a good time for js::{Thin,Skinny,ProbablySmall}Vector.

Comment 3

[Nicholas Nethercote [inactive]]

dvander tells me that PICs and MICs used to be flushed at different times, which is why each PICInfo (and each GetElementIC) has an execPools (mini-)vector, and JITScript has an execPools for all the MICs.

But it seems that PICs (and GetElementICs) and MICs are now always flushed together at GCs, in which case PICInfo::execPools (BasePolyIC::execPools, actually) could be removed and JITScript::execPools could be changed to hold pools used by both PICs and MICs.  JITScript::execPools could be a mini-vector, if measurements show that 0 or 1 elements still dominates.

dvander:  please let me know if I've got these details wrong!

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> 
> But it seems that PICs (and GetElementICs) and MICs are now always flushed
> together at GCs, in which case PICInfo::execPools (BasePolyIC::execPools,
> actually) could be removed and JITScript::execPools could be changed to hold
> pools used by both PICs and MICs.

AFAICT, this is wrong.

In order to move the execPools from BasePolyIC into JITScript, it requires that BasePolyIC::releasePools() only be called from ~BasePolyIC() -- because ~BasePolyIC() is called from ~JITScript(), ie. each BasePolyIC has the same end-of-lifetime as its parent JITScript.

But BasePolyIC::releasePools() is also called from BasePolyIC::reset(), which is called from PICInfo::reset(), which is called from JITScript::purgePICS(), which is called from ic::PurgePICs(), which is called from JSCompartment::purge().

In other words, PICInfo's execPools can be released before JITScript is destroyed, so AFAICT we need to keep the current structure to avoid hanging onto executable pages containing PICs longer than necessary.

The situation is different for MICs because JITScript::purgeMICs() doesn't call any kind of MICInfo::reset() function.

Also, PurgePICs() is called on every compartment purge, whereas PurgeMICs() is only called if cx->runtime->gcRegenShapes is true.  So, to put it another way, PICs are flushed on every GC but MICs are not.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

interesting. I didn't bother instrumenting MIC data in the about:memory stuff, because I understood it to flush every GC.  how frequent are shape-regen GCs or purges?

Comment 6

[Nicholas Nethercote [inactive]]

Hmm, but every PICInfo within a JITScript is purged at the same time.  So we could have a PICInfo execPools vector within JITScript, it would just have to be separate from the MICInfo one, I think.  The tricky part would probably be to avoid duplicates in it.

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
> interesting. I didn't bother instrumenting MIC data in the about:memory stuff,
> because I understood it to flush every GC.

I think you did instrument it, because it's part of scriptDataSize().

> how frequent are shape-regen GCs or purges?

No idea.

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> (In reply to comment #5)
> > how frequent are shape-regen GCs or purges?
> 
> No idea.

Shape overflow is rare, 2^24 wraparound.

/be

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to comment #6)
> Hmm, but every PICInfo within a JITScript is purged at the same time.  So we
> could have a PICInfo execPools vector within JITScript, it would just have to
> be separate from the MICInfo one, I think.  The tricky part would probably be
> to avoid duplicates in it.

Ah, but recording the duplicates is necessary -- in ~JITScript() we have to decrement the reference count for each execPool the right number of times.  That sucks because it adds 24 or 32 bytes to JITScript for the vector, plus one word per execPool, plus wasted space due to the Vector doubling strategy, all just to remove one word from BasePolyIC.

Another possibility is to store a dup count for each execPool in a hash table, but the minimum hash table overhead will kill us there.

(I think the MIC execPools in JITScript has the same duplicate issue, BTW.)

So it sounds like going back to the original idea -- using a mini-Vector for JITScript::execPools -- might be best.  It'll save 20 bytes per JITScript on 32-bit and 32 bytes on 64-bit.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: WIP patch moving execPools from PICInfo to JITScript

Just for posterity.  Doesn't seem like a good idea.

Comment 11

[Jan de Mooij [:jandem]]

JM was removed.