684111 - Remove codePool and notePool

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

I noticed that there's some clownshoes behaviour in the arenas used by jsemit.cpp -- we allocate 2^N bytes for data but there's also an arena header, which causes jemalloc to round those allocations requests up, wasting space (from 1MB+epsilon to 2MB in extreme cases).  While investigating this, I realized that the arenas used by jsemit.cpp aren't even needed.  I have three patches that together remove them.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch 1

This patch just removes an unused parameter from EmitCheck.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch 2

|notes| always has 2^N-sized buffer that's mostly filled in one srcnote at a
time.  For some reason, instead of just recording the current buffer size
(e.g. 256), the code records a mask that is one less than the current buffer
size (e.g. 0xff) and then does bit-arithmetic with the mask to determine
when the note count reaches the limit.  

This patch just changes that mask to a max count, which makes things 
simpler.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch 3

This patch is the biggest.  Each JSCodeGenerator has two arena pools,
codePool and notePool.  But each JSCodeGenerator only allocates a single 
block from each of these pools, and resizes these chunks as necessary.  So 
why use arena pools at all?

Well, you can have nested JSCodeGenerators, and the way codePool and notePool
were being used was that each nested JSCodeGenerator got a subsequent chunk
from each arena pool.  But you don't need arena pools;  the lifetime nesting
implied by the call chain is sufficient -- each JSCodeGenerator can just use
malloc/realloc/free to manage its code block and its notes block.  (The
nested JSCodeGenerators were also allocated from the arena pools, but
vanilla new/delete suffices for them.)

This fixes the clownshoes and makes everything much simpler (including a net
reduction of 43 lines of code).  It also removes all but one use of
JS_ARENA_GROW_CAST, which is good because its implementation is pretty
horrible -- arena allocators and realloc-style allocation are an uneasy mix.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch 3b

This fixes a jstests assertion failure.

Comment 5

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

Comment on attachment 557706 [details] [diff] [review]
patch 2

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

Yuck.

r=me with LIMIT instead of MAX -- brendan has told me they have consistently distinguished meanings in the JS engine:

"MAX means last valid value, LIMIT means fencepost, so there's an off-by-one in the use of >= or else a misnomer..."
- bug 578272 comment 2

Comment 6

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

Comment on attachment 557760 [details] [diff] [review]
patch 3b

Nick, is there a reason we shouldn't use js::Vector for these heapy alloc doubling things? I'm not seeing one but I think I could be missing it. I figure having inline storage could be helpful for avoiding allocations in the small script case.

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to Chris Leary [:cdleary] from comment #5)
> 
> r=me with LIMIT instead of MAX -- brendan has told me they have consistently
> distinguished meanings in the JS engine:

For the bytecode we have a |limit| field, but it's a pointer.  And this is a count, so I deliberately didn't use the same terminology.  Does the usual meaning of "limit" apply to both pointers and counts?

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Chris Leary [:cdleary] from comment #6)
> 
> Nick, is there a reason we shouldn't use js::Vector for these heapy alloc
> doubling things?

No reason, I just didn't think of that.  I'll do some measurements to see if there's a good small inline size that would cover a decent fraction of the cases.

Comment 9

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

(In reply to Nicholas Nethercote [:njn] from comment #7)
> For the bytecode we have a |limit| field, but it's a pointer.  And this is a
> count, so I deliberately didn't use the same terminology.  Does the usual
> meaning of "limit" apply to both pointers and counts?

Yeah, limit pointer in that case is one-past-the-last-valid bytecode location. When CG_BASE(cg) == CG_LIMIT(cg), you have to expand the bytecode buffer in order to add another. I guess they called it CG_LIMIT instead of CG_BYTECODE_LIMIT for brevity.

I just meant that CG_NOTE_LIMIT (and a corresponding noteLimit field) were more in line with the SpiderMonkey usage; i.e. when CG_NOTE_COUNT(cg) == CG_NOTE_LIMIT(cg) you have to expand in order to add another.

In any case, all of this terminology nonsense makes me want to use vectors even more, you get to avoid everything and just use appendN! Even if we can't get inline storage that makes sense, letting js::Vector manage all the necessary alloc expansions should cut down on the lines of code here a bit.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: partial vector-ification patch

> Yeah, limit pointer in that case is one-past-the-last-valid bytecode
> location. When CG_BASE(cg) == CG_LIMIT(cg), you have to expand the bytecode
> buffer in order to add another. I guess they called it CG_LIMIT instead of
> CG_BYTECODE_LIMIT for brevity.
> 
> I just meant that CG_NOTE_LIMIT (and a corresponding noteLimit field) were
> more in line with the SpiderMonkey usage; i.e. when CG_NOTE_COUNT(cg) ==
> CG_NOTE_LIMIT(cg) you have to expand in order to add another.

I'm repeating myself, but if I do that then CG_BYTE_LIMIT and CG_NOTE_LIMIT will mean different things (one is a pointer limit, the other is an index limit).  Maybe that's ok, though.

> In any case, all of this terminology nonsense makes me want to use vectors
> even more, you get to avoid everything and just use appendN! Even if we
> can't get inline storage that makes sense, letting js::Vector manage all the
> necessary alloc expansions should cut down on the lines of code here a bit.

I tried this, unfortunately it's difficult.  I've attached the patch that converts the bytecode, it works except that that the "if (constPropagated)" case in EmitSwitch plays funny buggers with CG_NEXT(cg) and I couldn't work out how to modify that to work with vectors.  I've attached the patch for future reference.

I also tried converting the notes, but the existing code does almost non-stop non-trivial pointer arithmetic that I didn't have the stomach to futz with, esp. given the lack of solution for the EmitSwitch case above.

Given the above, would you be willing to review my patch 3b?  It is a strict improvement over the status quo (due to the removal of the unnecessary arena usage, the replacement of the counts mask with a limit, and the avoiding of the wasted space).  Also, the vector-ification is major scope creep for this bug. And finally, I'm going on vacation for a month at the end of this week and I'd like to get it in before then if possible so that the improvements make it into FF9.

I'm happy to file a follow-up bug to Vector-ify everything.

Comment 11

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

(In reply to Nicholas Nethercote [:njn] from comment #10)
> will mean different things (one is a pointer limit, the other is an index
> limit).  Maybe that's ok, though.

Yeah, I was saying that the mathematical concept of "limit" is the same across those two cases, despite the different datatypes.

> Given the above, would you be willing to review my patch 3b?

Yes.

> Also, the vector-ification is major scope creep for this
> bug.

If it was easy I didn't think it was creepy, but since it's hard I agree.

> I'm happy to file a follow-up bug to Vector-ify everything.

Please do!

Comment 12

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

Comment on attachment 557760 [details] [diff] [review]
patch 3b

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

Cool beans.

::: js/src/jsemit.cpp
@@ +167,4 @@
>          if (!base) {
> +            JS_ASSERT(!next && !limit);
> +            newlength = BYTECODE_CHUNK_LENGTH;
> +            if (newlength < minlength)     // make it bigger if necessary

Nit: looks like /* comments in this file */

@@ +167,5 @@
>          if (!base) {
> +            JS_ASSERT(!next && !limit);
> +            newlength = BYTECODE_CHUNK_LENGTH;
> +            if (newlength < minlength)     // make it bigger if necessary
> +                newlength = JS_BIT(JS_CeilingLog2(minlength));

I think you want RoundUpPow2 in jstl.h

@@ +173,5 @@
>          } else {
> +            JS_ASSERT(base <= next && next <= limit);
> +            newlength = (limit - base) * 2;
> +            if (newlength < minlength)     // make it bigger if necessary
> +                newlength = JS_BIT(JS_CeilingLog2(minlength));

Ditto.

::: js/src/jsemit.h
@@ +661,5 @@
>      uint16          typesetCount;   /* Number of JOF_TYPESET opcodes generated */
>  
>      /*
> +     * cg allocates bytecode space and source note space itself, and all other
> +     * arena-allocated temporaries from parser->context->tempPool.

I think we should just kill this comment now, I don't think it is (or was) very helpful.

::: js/src/jsparse.cpp
@@ +1840,5 @@
>                  pn = NULL;
>          }
>      }
>  
> +    /* Restore saved state. */

I don't think this comment applies anymore, we just return a status code here.

Comment 13

[Brendan Eich [:brendan]]

Thanks for cleaning up very old undermaintained code. A couple of notes/nits:

1. The EmitCheck op parameter was used, cvs.mozilla.org rev 3.1 shows this. I didn't bother to find where its use in the body went away.

2. Naming nit: for power of two lengths, mask is max -- not limit; max == maximum value in domain, limit == fencepost, do not equal or exceed. <limits.h> _MAX usage matches this, we should not start using MAX or max or Max as a name part for fenceposts or limit constants. So notesLimit/CG_NOTES_LIMIT is consistent with other "limit"-based names in jsemit.cpp (and the rest of js/src).

/be

Comment 14

[Brendan Eich [:brendan]]

(In reply to Brendan Eich [:brendan] from comment #13)
> notesLimit/CG_NOTES_LIMIT is consistent with other "limit"-based names in

s/notes/note/g, of course.

And apologies for picking the LIMIT scab, I was reading old bugmail and should have read comment 9 and on. Using "limit" for a pointer or an index is fine, the point is it's the asymptote or fencepost. In Unix kernel and other work, I've found that mixing up MAX and LIMIT leads to off-by-ones. Since SpiderMonkey is pretty consistent still in using MAX for greatest value in domain and LIMIT for fencepost, I think we should keep consistent.

/be

Comment 15

[Nicholas Nethercote [inactive]]

(In reply to Chris Leary [:cdleary] from comment #11)
> 
> > Also, the vector-ification is major scope creep for this bug.
> 
> If it was easy I didn't think it was creepy, but since it's hard I agree.

True!

> > I'm happy to file a follow-up bug to Vector-ify everything.
> 
> Please do!

Bug 685078.


Inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/054b9cf30cd1
http://hg.mozilla.org/integration/mozilla-inbound/rev/4c0087b808de
http://hg.mozilla.org/integration/mozilla-inbound/rev/5ddb67d5a424

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/054b9cf30cd1
http://hg.mozilla.org/mozilla-central/rev/4c0087b808de
http://hg.mozilla.org/mozilla-central/rev/5ddb67d5a424