787246 - rm OffTheBooks/Foreground/UnwantedForeground gunk

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: rm

The OffTheBooks alloc functions were added under the misunderstanding that it was always better to account for allocation than not.  This isn't true: allocations should only be accounted in gcMallocBytes if they would could be freed by doing a GC (e.g., the private bytes of a GC-thing).

The Foreground/UnwantedForeground free functions were added under the misunderstanding that all frees will be done on the background thread.  This isn't true: only those during GC (this is now statically enforced: no JSContext is available during finalization, only a FreeOp which handles the background free).

This patch removes this confusing gunk.  It also removes array_new/array_delete because they are almost never used and, when they are, malloc would have been fine (and more efficient in time/space) since there is no destructor.  (We usually use Vector.)

Here's the comment that replaces the existing one in Utility.h:

/*
 * Low-level memory management in SpiderMonkey:
 *
 *  ** Do not use the standard malloc/free/realloc: SpiderMonkey allows these
 *     to be redefined (via JS_USE_CUSTOM_ALLOCATOR) and Gecko even #define's
 *     these symbols.
 *
 *  ** Do not use the builtin C++ operator new and delete: these throw on
 *     error and we cannot override them not to.
 *
 * Allocation:
 *
 * - If the lifetime of the allocation is tied to the lifetime of a GC-thing
 *   (that is, finalizing the GC-thing will free the allocation), call one of
 *   the following functions:
 *
 *     JSContext::{malloc_,realloc_,calloc_,new_}
 *     JSRuntime::{malloc_,realloc_,calloc_,new_}
 *
 *   The difference between the JSContext and JSRuntime versions is that the
 *   cx version reports an out-of-memory error on OOM. (This follows from the
 *   general SpiderMonkey idiom that a JSContext-taking function reports its
 *   own errors.)
 *
 * - Otherwise, use js_malloc/js_realloc/js_calloc/js_free/js_new
 *
 * Deallocation:
 *
 * - Ordinarily, use js_free/js_delete.
 *
 * - For deallocations during GC finalization, use one of the following
 *   operations on the FreeOp provided to the finalizer:
 *
 *     FreeOp::{free_,delete_}
 *
 *   The advantage of these operations is that the memory is batched and freed
 *   on another thread.
 */

Comment 1

[Luke Wagner [:luke]]

Attachment: patch

Found bugs re-reading the patch.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 657092 [details] [diff] [review]
patch

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

::: js/public/Utility.h
@@ +389,4 @@
>   *
> + * - If the lifetime of the allocation is tied to the lifetime of a GC-thing
> + *   (that is, finalizing the GC-thing will free the allocation), call one of
> + *   the following functions:

Can you briefly explain (in the comment) why these functions should be used instead of js_malloc et al?  Presumably it's because of the accounting.

@@ +401,2 @@
>   *
> + * - Otherwise, use js_malloc/js_realloc/js_calloc/js_free/js_new

The fact that the cx/rt versions should be used with GC-tied things and the js_malloc versions should be used otherwise isn't at all clear from the names.  Kind of a shame.  I liked OffTheBooks' clarity, even if it wasn't quite right.

TiedToGC::new vs NotTiedToGC?  Eh...

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 657092 [details] [diff] [review]
patch

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

Excellent!

::: js/src/jsapi.h
@@ +6006,3 @@
>      const char      *linebuf;       /* offending source line without final \n */
>      const char      *tokenptr;      /* pointer to error token in linebuf */
>      const jschar    *uclinebuf;     /* unicode (original) line buffer */

I would almost suggest stripping off the const qualifiers here so that we're allowing to free these things without a cast. But maybe I'm being overzealous.

::: js/src/jsexn.cpp
@@ +1190,3 @@
>              }
>          }
>      } autoFree = {cx, copy};

Can we replace this class with a use of AutoReleasePtr?

Comment 5

[Luke Wagner [:luke]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Can you briefly explain (in the comment) why these functions should be used
> instead of js_malloc et al?  Presumably it's because of the accounting.

Oops, yes, meant to do that.

> Eh...

Yeah, that's how I feel.  A single <10 character name is not going to convey the entire meaning, no matter what you call it; you just need to read the comment.  Once you know the meaning, it's not hard to remember which to use.  Hence, I think it is right to prefer terseness in this case.

(In reply to Bill McCloskey (:billm) from comment #4)
> I would almost suggest stripping off the const qualifiers here so that we're
> allowing to free these things without a cast. But maybe I'm being
> overzealous.

Normally I would, but I think it belongs since this is the public API.

> Can we replace this class with a use of AutoReleasePtr?

Well, you need to add AutoReleasePtr::forget() and use two of them (one for p, one for p->errorReport), but 'yes', I will.

Comment 6

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0789db68c77

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0789db68c77