Closed Bug 787246 Opened 8 years ago Closed 8 years ago

rm OffTheBooks/Foreground/UnwantedForeground gunk

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch rm (obsolete) — Splinter Review
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.
 */
Attachment #657068 - Flags: review?(wmccloskey)
Attached patch patchSplinter Review
Found bugs re-reading the patch.
Attachment #657068 - Attachment is obsolete: true
Attachment #657068 - Flags: review?(wmccloskey)
Attachment #657092 - Flags: review?(wmccloskey)
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...
Blocks: 787291
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?
Attachment #657092 - Flags: review?(wmccloskey) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/d0789db68c77
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.