Open Bug 646044 Opened 9 years ago Updated 5 years ago

ContextAllocPolicy should not account for GC memory pressure

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

People

(Reporter: igor, Unassigned)

Details

Attachments

(1 file, 2 obsolete files)

Currently ContextAllocPolicy uses JSContext::(malloc|realloc|free) to implement the memory allocation functions. Many if not all of the users of the policy allocate temporary memory that is released when the corresponding native method returns. As such there is no GC pressure that can increase or the memory cannot be released by the GC on the background thread. For such classes we should provide ReportedAllocPolicy that uses js_(malloc|realloc|free) and explicitly reports the OOM. For the rest of the cases if any we should provide GCPressureAllocPolicy.
Makes sense
After checking all our code I have not found any needs for GCPressureAllocPolycy. That is, all users of ContextAllocPolicy are of temporary nature. Thus for the fix it is sufficient to implement ContextAllocPolicy in terms of js_(malloc|realloc|free).

This also suggest to rename JSContext::(malloc|realloc|free) into JSContext::(malloc|realloc|free)ForGC and add allocations functions like js_malloc(JSContext *cx) that would do the error reporting. But this is for another bug.
Summary: Split ContextAllocPolicy into ReportingAllocPolicy and GCAllocPolicy → ContextAllocPolicy should not account for GC memory pressure
Attached patch v1 (obsolete) — Splinter Review
The patch removes GC pressure accounting from ContextAllocPolicy.
Attachment #522881 - Flags: review?(luke)
So, as you might've seen on the mailing list, Paul just landed a bunch of allocation interface changes.  Your changes here seem to defy the current three-level classification scheme (http://hg.mozilla.org/tracemonkey/file/bd821ea0ad41/js/src/jsutil.h#l262): what we have here is an allocation policy where we want reporting but not bookkeeping.  Given that JSContext::malloc_ does reporting *and* bookkeeping, it seems that, at least, your patch should rename ContextAllocPolicy (to TempAllocPolicy, or something) since the name implies we're just calling cx->malloc_.

Going further, and this might be too weird, we could introduce a 4th set of malloc_/free_/etc functions that capture this "temporary" allocation strategy (reported, no bookkeeping).  One wacky idea is to add:

  JSRuntime::malloc_(JSContext *cx, size_t bytes);
  ... etc

which does no bookkeeping (like the current JSRuntime::malloc_) but does report errors (following the convention that functions that take a JSContext* as the first arg report errors).  What say you?
Attachment #522881 - Flags: review?(luke)
Attached patch v2 (obsolete) — Splinter Review
The new version is updated for the recent allocation changes. Compared with the previous version the patch waits for background free to finish and retry the allocation before reporting OOM. I will use this for the bug 647103 to provide a temporary allocator.
Attachment #522881 - Attachment is obsolete: true
Attachment #524254 - Flags: review?(luke)
Why make a new jsalloc.h/.cpp instead of just using jsutil.h/.cpp where all the existing malloc_ and friends live?

Also, the name "ContextAllocPolicy" and "SystemAllocPolicy" seem more and more out of date with all the fresh new names that have clear semantics attached.  Are you planning to rename ContextAllocPolicy/SystemAllocPolicy with bug 647103?
(In reply to comment #6)
> Why make a new jsalloc.h/.cpp instead of just using jsutil.h/.cpp where all the
> existing malloc_ and friends live?

I do not want to add more stuff to a public header for now.

> Also, the name "ContextAllocPolicy" and "SystemAllocPolicy" seem more and more
> out of date with all the fresh new names that have clear semantics attached
> Are you planning to rename ContextAllocPolicy/SystemAllocPolicy with bug
> 647103?

Yes, I want to rename it in the bug 647103.
Attached patch v3Splinter Review
Here is an updated patch
Attachment #524254 - Attachment is obsolete: true
Attachment #524254 - Flags: review?(luke)
Attachment #525661 - Flags: review?(luke)
Attachment #525661 - Flags: review?(luke) → review+
Assignee: igor → general
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.