415300 - jsiter and jsinterp access JSArena code directly, break encapsulation

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect, P5)

Description

[Robin Bate Boerop]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/523.12.2 (KHTML, like Gecko) Version/3.0.4 Safari/523.12.2
Build Identifier: 

jsiter.* and jsinterp.* and maybe other code in js/src try to manually add new JSArenas to JSArenaPools.  This causes waste, because a prior arena may not be full before this happens.  More importantly, it makes changing the algorithms for JSArenas very difficult, as it depends on the layout of JSArena-related structures in memory.

We need to find another solution to the problem being solved when this was done by jsiter and jsinterp code.



Reproducible: Always

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en)
> AppleWebKit/523.12.2 (KHTML, like Gecko) Version/3.0.4 Safari/523.12.2

Nice :-P.

> jsiter.* and jsinterp.* and maybe other code in js/src try to manually add new
> JSArenas to JSArenaPools.  This causes waste, because a prior arena may not be
> full before this happens.

That's a false claim, AFAICT. Can you prove it?

> More importantly, it makes changing the algorithms
> for JSArenas very difficult, as it depends on the layout of JSArena-related
> structures in memory.

This is a sign of a small, engine-internal dependency on implementation details that are exposed via jsarena.h. It's not merely a leaky abstraction -- it's intentional reuse of a data structure for a more specialized task than fits in jsarena.h.

Therefore to avoid regressing performance, we should consider alternative stack representations. Putting arena metadata elsewhere could be good for cache hit rates, but it could increase miss rates if the "elsewhere" collides with hot stack items.

Some rethinking is needed here.

/be

Comment 2

[Robin Bate Boerop]

(In reply to comment #1)
> (In reply to comment #0)
> > User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en)
> > AppleWebKit/523.12.2 (KHTML, like Gecko) Version/3.0.4 Safari/523.12.2
> Nice :-P.
Burn!  Safari is still faster on my laptop.  :-P

> > jsiter.* and jsinterp.* and maybe other code in js/src try to manually add new
> > JSArenas to JSArenaPools.  This causes waste, because a prior arena may not be
> > full before this happens.
> That's a false claim, AFAICT. Can you prove it?
Working on it.

> > More importantly, it makes changing the algorithms
> > for JSArenas very difficult, as it depends on the layout of JSArena-related
> > structures in memory.
> This is a sign of a small, engine-internal dependency on implementation details
> that are exposed via jsarena.h. It's not merely a leaky abstraction -- it's
> intentional reuse of a data structure for a more specialized task than fits in
> jsarena.h.
The data structure in question, JSArena, was not really meant to be externalized to begin with.  It is an implementation detail of JSArenaPools. I do agree that the code which manipulates JSArenas in jsinterp.c does implement the logic that you want for stack frames.  I can implement some simple functions which will avoid the direct access of JSArena, and allow me to change the logic that I need to change.  However, the code which manipulates the JSArenas in jsiter.c is not quite right, I think.

> Therefore to avoid regressing performance, we should consider alternative stack
> representations. Putting arena metadata elsewhere could be good for cache hit
> rates, but it could increase miss rates if the "elsewhere" collides with hot
> stack items.
Putting stack meta data elsewhere makes most sense when the arena is backed by a malloc()d allocation.  Each arena is backed by a malloc()d allocation, except in the jsiter.c, where the code constructs arenas manually from gen->arena.  I don't know where the memory for those (gen) generators comes from.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> > That's a false claim, AFAICT. Can you prove it?
> Working on it.

Look at AllocateAfterSP. Tell me where it wastes space, apart from the non-waste case where nslots cannot be allocated contiguously (for vars or argv vectors).

> The data structure in question, JSArena, was not really meant to be
> externalized to begin with.  It is an implementation detail of JSArenaPools.

Hey, I wrote this code, I get to decide. It's low level C code. JSArena is exported in the .h file for use by macros, and rather than reinvent that wheel, I used it elsewhere. I think you are polishing something that does not need to be polished.

> I
> do agree that the code which manipulates JSArenas in jsinterp.c does implement
> the logic that you want for stack frames.  I can implement some simple
> functions which will avoid the direct access of JSArena, and allow me to change
> the logic that I need to change.

Let's agree on making the change you proposed for bug 408921 first.

If we do agree to abstract JSArena, the function overhead and code costs should not go up. This could be a simple as making AllocateAfterSP a proper jsarena.h API, but there's no point if we aren't going to move the metadata out of line.

> However, the code which manipulates the
> JSArenas in jsiter.c is not quite right, I think.

It's correct (there's no malfunction). That it does not use space under gen->arena after linking that JSArena to cx->stackPool.current->next and making it current is, again, intentional. If you can find a way to avoid this without copying the stack on every SendToGenerator, I'm all ears. Copying a small-enough stack might be ok (although pointer fixup is required). Until profiling shows a real space or time bug here, though, I claim we have bigger fish to fry.

> Putting stack meta data elsewhere makes most sense when the arena is backed by
> a malloc()d allocation.

That's a nice assertion, but it has nothing to do with the cache effectiveness argument from bug 408921, and it needs proof. If we can adjust net sizes and keep metadata contiguous, then we don't need to add more code and take risks with runtime costs -- and probably lose data cache effectiveness (see the comment I'm about to make in bug 408921) -- just to satisfy this "makes the most sense".

This is not a matter of sense or logic, it's a matter of performance.

> Each arena is backed by a malloc()d allocation, except
> in the jsiter.c, where the code constructs arenas manually from gen->arena.  I
> don't know where the memory for those (gen) generators comes from.

jsiter.c is a small-ish file. There are exactly five hits searching for the /gen = / pattern, and the third one, in js_NewGenerator (obvious name given the prevailing conventions) allocates the generator and frame.

/be

Comment 4

[Robin Bate Boerop]

(In reply to comment #3)
> (In reply to comment #2)
> > > That's a false claim, AFAICT. Can you prove it?
> > Working on it.
> Look at AllocateAfterSP. Tell me where it wastes space, apart from the
> non-waste case where nslots cannot be allocated contiguously (for vars or argv
> vectors).

This was about my claim that "code in js/src try to manually add new JSArenas to JSArenaPools.  This causes waste, because a prior arena may not be full before this happens."  This does not happen in AllocateAfterSP(), but in jsiter.c in SendToGenerator():
   http://lxr.mozilla.org/seamonkey/source/js/src/jsiter.c#875

There, the "current" arena is replaced by the JSArena in a JSGenerator.  I logged how much space was in the replaced (wasted) arena.  It was ~8K when I made it happen with some JavaScript:
   (function () { yield })().send()

That is sufficient to prove my claim.

A single 8K waste is no big deal, especially if it is quickly undone, but since I don't know the code well, I can't tell if this might happen many times.  If this were happening recursively, many arenas may be wasted.  The space wasted might be significantly large.

Like I said, I don't know the code well enough to see if this happens recursively.  Maybe someone can point me to a workload that might stress this particular code path.  Or, maybe it's obvious to someone who knows the code that this can never happen more than once.  Can anyone help?

Comment 5

[Robin Bate Boerop]

(In reply to comment #3)
> (In reply to comment #2)
> > The data structure in question, JSArena, was not really meant to be
> > externalized to begin with.  It is an implementation detail of JSArenaPools.
> Hey, I wrote this code, I get to decide. It's low level C code. JSArena is
> exported in the .h file for use by macros, and rather than reinvent that wheel,
> I used it elsewhere.

Fair enough.  Sorry, Brendan.  When I said that arenas were not meant to be externalized, I was thinking of the arenas in the original paper by David Hanson (http://storage.webhop.net/documents/fastalloc.pdf).

Comment 6

[Robin Bate Boerop]

(In reply to comment #3)
> (In reply to comment #2)
> > Putting stack meta data elsewhere makes most sense when the arena is backed by
> > a malloc()d allocation.
> That's a nice assertion, but it has nothing to do with the cache effectiveness
> argument from bug 408921, and it needs proof. If we can adjust net sizes and
> keep metadata contiguous, then we don't need to add more code and take risks
> with runtime costs -- and probably lose data cache effectiveness (see the
> comment I'm about to make in bug 408921) -- just to satisfy this "makes the
> most sense".
> 
> This is not a matter of sense or logic, it's a matter of performance.

I should have been more clear with my assertion.  I should have said "because the arenas are NOT backed by a malloc()d allocation, my arguments for changing where the stack (arena) meta data are kept do not all hold."  I don't think we disagree on this matter.

That said, we can't easily adjust the net sizes, for reasons that David Crowder gives in bug 408921.  Also, there is a better (and synergistic) way to handle the gross/net size discrepancy anyway - see the next bug I open.

Comment 7

[Robin Bate Boerop]

I would like to use this bug to move the code that pushes the arena onto the list of arenas.  I want to move it to jsarena.?.  That should make it easier to do my memory allocation gathering and stats-gathering work, without "forgetting" about the arenas created and manipulated outside of jsarena code.

Basically, I'll just provide a function or macro that pushes the already-init'd arena onto the list.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Is this bug still relevant 3+ years later?

Comment 9

[Robin Bate Boerop]

(In reply to comment #8)
> Is this bug still relevant 3+ years later?

Highly unlikely. Resolving.