647103 - Reporting allocator and better names for malloc classes.

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

This is a followup for the bug 643548 comment 16.

Currently SM does not provide a helper API to allocate/free memory using the system malloc/free while reporting OOM ti the JSContext instance. This lead to incorrect usage of JSContext::(malloc|free). JSContext::malloc should be used only to allocate the memory that would be stored in GC things and released from the finalizer.

But, since JSContext::malloc also reports OOM, the code also uses it as a convenience to allocate temporary storage that will be released when a particular native returns. This not only unduly influences the GC memory pressure counter but also leads to an incorrect usage of JSContext::free that would do a useless check for the background free facilities. 

The bug 643548 has added to the confusion with comments that encourage using cx->malloc_ and names like OffBooks::malloc_ and Foreground::free_ for the system allocation functions. Another problem with this newly introduced names is that they tell about the implementation but do not provide useful clues about the intended usage.

So to clear this I suggest to replace OffBooks and Foreground with single SysMem, move JSContext::malloc/free into separated GCMem class. We should also add a class like TempMem that would provide a version of SysMem that reports OOM on the supplied JSContext.

Comment 1

[Paul Biggar]

(In reply to comment #0)
> Another problem with this newly introduced names
> is that they tell about the implementation but do not provide useful clues
> about the intended usage.
> 
> So to clear this I suggest to replace OffBooks and Foreground with single
> SysMem

I think this is a step back - we learn nothing at all from the name SysMem, which is exactly why they were split into Foreground and OfftheBooks. What does SysMem say about the intended usage?

> , move JSContext::malloc/free into separated GCMem class. We should also
> add a class like TempMem that would provide a version of SysMem that reports
> OOM on the supplied JSContext.

Beware that some of the interfaces you might want to create are difficult to create in practice (for example, adding a |cx| param at the start of another param list).


I think we need a fully fleshed out proposal to evaluate it (it took several attempts to get this right in previous bugs). Could you provide a spec for each new class, with it's functions and parameters, and a mapping from the current allocation/deallocation functions?

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> I think this is a step back - we learn nothing at all from the name SysMem,
> which is exactly why they were split into Foreground and OfftheBooks. What does
> SysMem say about the intended usage?

SysMem tells that one should think twice before using it. In general GCMem or TempMem should be used instead.

> I think we need a fully fleshed out proposal to evaluate it (it took several
> attempts to get this right in previous bugs). Could you provide a spec for each
> new class, with it's functions and parameters, and a mapping from the current
> allocation/deallocation functions?

The GCMem class should be used only to allocate memory that will be stored in the GC things that will be released either in the finalizer or on a failure path. The last may happen when one failed to allocate JSString after allocating the string char array. This is a replacement for JSContext:: methods. It may look like:

class GCMem {
  public:
    static void *malloc_(JSContext *cx, size_t size);
    static void *free_(JSContext *cx, void *p);
    static template<typename T> T* new_(JSContext *cx);
    static template<typename T, typename T1> T* new_(JSContext *cx, T1 arg);
    // other new forms
    static template<typename T> void delete_(JSContext *cx, T *p);
};

TempMem is for a temporary memory that is allocated and released when some native method is called by the engine. It will do the error reporting on OOM and, as GCMem, would wait until the background GC finishes if the initial allocation attempt fails.

class TempMem {
  public:
    static void *malloc_(JSContext *cx, size_t size);
    static void *free_(void *p);
    static template<typename T> T* new_(JSContext *cx);
    static template<typename T, typename T1> T* new_(JSContext *cx, T1 arg);
    static template<typename T> void delete_(T *p);
};

Compared with GCMem it free/delete methods do not take the cx param.

SysMem contains an aliases for system malloc/free and, perhaps, helpers to wait for the background GC to finish and restart the system allocation.

Any suggestions for better names are welcome. It would be nice to have names clearly capture the intended usage of the memory allocation API.

Comment 3

[Brendan Eich [:brendan]]

Is the goal of avoiding spurious GC memory pressure from purely stack-like allocations (LIFO and all managed via RAII auto-helpers or equivalent) clear? I buy it and agree with Igor that it's important not to penalize any of the three use-cases with overlong or spuriously asymmetric/different call site expressions. Making the three cases look similar via namespaces that distinguish them clearly also seems good. But how about

  System : GC : Auto (or Temp; best to avoid Stack)

instead? OffTheBooks is cute and may trump System but it's longer and what seems not to be shared by everyone is when and on which books one is "OnTheBooks".

/be

Comment 4

[Nicholas Nethercote [inactive]]

I liked Paul's names, and comment 0 feels like it's heading back in the direction of inscrutability.  "System" is meaningless.

I won't add anything more to the naming question, but I will note that Paul's patch had a lovely big comment that explained everything extremely clearly.  IMO that is more valuable than better names.  Whatever final design is decided on there, please please please have a decent comment describing how to use the various allocators.  (The trace JIT's allocators are an example here;  their names (dataAlloc, traceAlloc, tempAlloc, codeAlloc) aren't great but jscompartment.h has a nice comment explaining them.)

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> I liked Paul's names, and comment 0 feels like it's heading back in the
> direction of inscrutability.  "System" is meaningless.

OffTheBooks is meaningless IMO as well. As Brendan pointed out it is not clear which books. Plus OffTheBooks::alloc_ must be used with Foreground::free_ and that is rather odd looking. 

But lets not focus too much os the names for non-reporting and/or non-waiting-for-background-gc names and instead consider names for the GC and temporary allocations (the latter is absent in the current code). 

My initial thinking was to have two prefixes, GCMem:: and TempMem:: for that implemeted as classes. If I understand Brendan's comment he would prefer something like GC:: and Temp:: implemented as namespaces. Any other suggestions?

Comment 6

[Paul Biggar]

(In reply to comment #5)
 
> OffTheBooks is meaningless IMO as well.

I'm not sure why.  OffTheBooks means not on any books (aka unaccounted).

> As Brendan pointed out it is not clear
> which books.

The ones belonging to the passed context.


> Plus OffTheBooks::alloc_ must be used with Foreground::free_ and
> that is rather odd looking. 

In the comments, I say that they are not required to be used together. Was I mistaken?


> But lets not focus too much os the names for non-reporting and/or
> non-waiting-for-background-gc names and instead consider names for the GC and
> temporary allocations (the latter is absent in the current code). 

If TempMem doesn't do temporary allocations than I am confused.

 
> My initial thinking was to have two prefixes, GCMem:: and TempMem:: for that
> implemeted as classes. If I understand Brendan's comment he would prefer
> something like GC:: and Temp:: implemented as namespaces. Any other
> suggestions?

I think you should keep them as classes, as you wont get the friending with namespaces (I think). They should be within js:: of course. I guess the advantage of namespaces is that we can specify them for an entire .cpp file using 'using namespace', but I believe that this is counterproductive.

Comment 7

[Paul Biggar]

> SysMem contains an aliases for system malloc/free and, perhaps, helpers to wait
> for the background GC to finish and restart the system allocation.

Can you list the API for this?

Comment 8

[Igor Bukanov]

(In reply to comment #6) 
> > As Brendan pointed out it is not clear
> > which books.
> 
> The ones belonging to the passed context.

Currently OffTheBooks means do not account for the container of the global GC pressure while update the counters that the malloc implementation is using internally. For me as a non-native English this looks strange.

Besides there are proposals to use the system memory pressure to schedule the GC instead of our own GC counters. If we would do that then clearly OffTheBooks becomes misleading. So we would need to change the name again. This shows that the name is not future proof.

> > Plus OffTheBooks::alloc_ must be used with Foreground::free_ and
> > that is rather odd looking. 
> 
> In the comments, I say that they are not required to be used together. Was I
> mistaken?

Currently if one uses OffTheBooks::malloc, then Foreground::free should also be used. cx->free should only be used from the finalizers. 

Also as with OffTheBooks the name is not future proof. Consider if we would use a background free in jemalloc or switch the GC to call the free during allocation and not from the background thread.

> They should be within js:: of course. I guess the
> advantage of namespaces is that we can specify them for an entire .cpp file
> using 'using namespace', but I believe that this is counterproductive.

My preference is for classes exactly for this reason. We better avoid any possibility of writing free_/malloc_ anywhere in the sources.

> Can you list the API for this?

See JSContext::onOutOfMemory

Comment 9

[Brendan Eich [:brendan]]

Names are tertiary, comments are great when accurate and therefore secondary, agreement on semantics is primary. Do we have agreement yet? I still think not.

/be

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Names are tertiary, comments are great when accurate and therefore secondary,
> agreement on semantics is primary. Do we have agreement yet? I still think not.

After trying to implement the bug 648106 I have released that for now we do not need temporary allocators. 

With that bug fixed we would have clear separation between the memory released during the finalization when we have many free calls and the non-GC case when memory is allocated during some native method execution with the free calls that quickly follow the corresponding malloc. So if there are some code paths where the overhead of free harms user interactivity, then we should think about using arena-like allocations to avoid the malloc/free overhead entirely.

With these observations we need the following allocation/deallocation API:

1. Allocation for memory released during the GC. This is covered by cx->malloc_.

2. Allocation of other kind of memory. This is currently served by OffTheBooks::malloc_ but rather poorly. In particular, the caller is responsible for error reporting and there is no support to wait for the background GC to finish in case of OOM. This is what I would like to address in this bug and in the bug 646044.

3. The release of memory allocated in 1 during finalization. cx->free_ covers that. But we also have the cases when cx is not available so UnwantedForeground:: should be used for that (and only for that!). So for this bug I would like to make sure that UnwantedForeground:: is used only during finalization.

4. Release of memory in any other situations. Foreground:: covers that. 

2. Background release of the memory allocated in 1. This is covered by cx->free_.

Comment 11

[Luke Wagner [:luke]]

Is there any plan to continue this work Igor?  I think you have some pretty good points comment 10.  In bug 660734 comment 12 I was wanting to improve the *AllocPolicy situation (to remove the meaningless names and make the alloc policies more directly map to the allocation functions you're describing) and I came across a comment in jsalloc.h that pointed me here, so I was wondering if I should hold off or perhaps you were going to fix the *AllocPolicy's too?

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> Is there any plan to continue this work Igor?

With the background finalization the need for cx->free_ and friends becomes less important. I even suspect that we may remove them. If we start to do the GC according to the memory pressure, we would not need cx->malloc_ with its memeory pressure accounting. And if that would be removed, we can just use js_malloc and js_free everywhere. 

I will try to measure the need for the explicit background free during the GC and decided the fate of this bug after that.