Closed Bug 521390 Opened 15 years ago Closed 15 years ago

avoid checking for malloc memory pressure when allocating GC things from free lists

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 3 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
Bug 505315 indicates that the gcMallocBytes counter hurts performance since it is checked on each and every allocation og GC things even when using free lists. But as it is not clear when that bug will land and those counters complicates addressing the bug 505315 I suggest an alternative approach that should not affect any memory profile but still avoids the checks and would allow to simplify the GC allocator.

The idea is to check for memory pressure during allocation and when the gcMaxMallocBytes is hit, then clean the free lists on all threads. This way the GC thing allocation code would need to check for free things only on empty lists. Now, the price for this is complicated code in JSContext::updateMallocCounter. But currently that code is already complex and using a special helper method to both report an OOM error and trigger the GC would not make things worse (or more complex) for JSContex::malloc users while benefiting the GC allocator.

The patch below implements this idea. It does not affect SunSpider benchmark but for v8 it makes things faster by about 1.5%.
Assignee: general → igor
No longer blocks: 505315
Attached patch v2 (obsolete) — Splinter Review
Here is the updated patch that passes the tests. The reason v1 failed on some  xpcshell-tests is that it updated the malloc counter on each realloc, not only on realloc that allocates from zero to something, as it is done without the patch. That triggered an extra GC that the test suit does not like. v2 restores that.
Attachment #405425 - Attachment is obsolete: true
Attachment #406411 - Flags: review?(brendan)
Attached patch v3 (obsolete) — Splinter Review
The new version syncs the patch with the tip changes due to the bug 505315.
Attachment #406411 - Attachment is obsolete: true
Attachment #406651 - Flags: review?(brendan)
Attachment #406411 - Flags: review?(brendan)
Comment on attachment 406651 [details] [diff] [review]
v3

>Index: tm/js/src/jsapi.cpp
>===================================================================
>--- tm.orig/js/src/jsapi.cpp	2009-10-16 11:47:47.000000000 +0400
>+++ tm/js/src/jsapi.cpp	2009-10-16 11:59:06.953762406 +0400
>@@ -2493,17 +2493,17 @@ JS_MaybeGC(JSContext *cx)
>      *   F == 0 && B > 3/2 Bl(1-Fl)
>      * Again using the stats we see that Fl is about 11% when the browser
>      * starts up and when we are far from hitting rt->gcMaxBytes. With
>      * this F we have
>      * F == 0 && B > 3/2 Bl(1-0.11)
>      * or approximately F == 0 && B > 4/3 Bl.
>      */
>     if ((bytes > 8192 && bytes > lastBytes + lastBytes / 3) ||
>-        rt->gcMallocBytes >= rt->gcMaxMallocBytes) {
>+        rt->gcMallocBytes < 0) {

Could comment the novel rt->gcMallocBytes < 0 condition, but better to encapsulate it with a JSRuntime inline method, say JSRuntime::maxMallocBytesExceeded() or something like that.

>+void
>+JSContext::checkMallocAllocation(void *p)

This "MallocAlloc..." stuttering is redundant and hard to say/read -- how about "checkForOutOfMemory" or "checkMaxMallocBytes"?

>+    JS_ASSERT(thread->gcThreadMallocBytes < 0);
>+    ptrdiff_t n = JS_GC_THREAD_MALLOC_LIMIT - thread->gcThreadMallocBytes;
>+    thread->gcThreadMallocBytes = JS_GC_THREAD_MALLOC_LIMIT;
>+
>+    JS_LOCK_GC(runtime);
>+    runtime->gcMallocBytes -= n;
>+    if (runtime->gcMallocBytes < 0)

Should this test < or <= ?

>+#endif
>+    {
>+        JS_ASSERT(runtime->gcMallocBytes < 0);
>+        runtime->gcMallocBytes = -1;
>+
>+        /*
>+         * Empty the GC free lists to trigger the last-ditch GC when
>+         * Empty the GC free lists to trigger a last-ditch GC when allocating

Nit: s/the last-ditch/a last-ditch/ and rewrap.

>+         * allocating any GC thing later on this thread. Note that we cannot
>+         * touch the free lists on other threads as their manipulation is not
>+         * thread-safe.

Might want to say that this arrangement minimizes tests in the allocator.

>+
>+    /*
>+     * Malloc counter to messure the memory pressure for GC scheduling. It

"measure" typo.

No need for "the" before "memory pressure".

>+     * runs from gcMaxMallocBytes down to zero.
>+     */
>+    ptrdiff_t           gcMallocBytes;
>+
>+    /*
>+     * Stack of the GC arenas with GC things that the GC has marked but has

"Stack of GC arenas" (no "the" again), and "containing" beats "with" here. Note "GC things" in "with" phrase is then referenced here by "they", which is ungrammatical:

>+     * not yet scanned for the GC things they refer to. It is used to avoid
>+     * recursive GC marking with a low native stack.

Maybe reword to say "Stack of GC arenas containing things that the GC marked, where children reached from those things have not yet been marked. This is used to avoid using too much stack during recursive GC marking."

>+    ptrdiff_t & getMallocCounter() {

Nit: no space after &.

>+    /*
>+     * Call this after malloc of memory for GC-related things to update memory

Suggest "Call this after allocating memory held by GC things, to update memory...."

>+    /*
>+     * Call this after successful malloc of memory for GC-related things.

Ditto, sort of.

>+private:
>+
>+    /*
>+     * The allocation code calls the function to indicate either OOM failure
>+     * when p is null or that a malloc pressure counter has reached some

s/malloc/memory/

Should we try to account for realloc growth bytes finally (separate bug)? I'm not so concerned about API compat as about missing memory pressure feedback.

r=me with nits picked, thanks.

/be
Attachment #406651 - Flags: review?(brendan) → review+
Attached patch v4Splinter Review
Besides addressing the nits in the new patch I also fixed few places where I missed to reset gcMallocBytes after changes in gcMaxMallocBytes.
Attachment #406651 - Attachment is obsolete: true
Attachment #406861 - Flags: review?(brendan)
Blocks: 522867
Comment on attachment 406861 [details] [diff] [review]
v4

>+     * Stack of GC arenas containing things that the GC marked, where children
>+     * reached from those things have not yet been marked. This is used to
>+     * avoid using too much native stack during recursive GC marking."

Stray " at end of last line.

On second read, "used to avoid using" overuses "use" -- how about "This helps avoid using too much ...."

r=me with that. Thanks for catching more than I did last time!

/be
Attachment #406861 - Flags: review?(brendan) → review+
http://hg.mozilla.org/tracemonkey/rev/37cfb3befe00
Whiteboard: fixed-in-tracemonkey
This seems to have caused a Talos regression. I now have two of these to look into. I guess I'll need to look at discrete data to make any progress.

-----

Regression: Dromaeo increase 3.03% on Leopard TraceMonkey

Previous results: 11.9515 from build 20091018083540 of revision b03e2edfedb6 at 2009-10-18 09:11:00 on talos-rev2-leopard07 run # 0

New results: 12.3132 from build 20091018085415 of revision 37cfb3befe00 at 2009-10-18 11:22:00 on talos-rev2-leopard14 run # 0

Suspected checkin range: from revision b03e2edfedb6 to revision 37cfb3befe00
(In reply to comment #7)
> This seems to have caused a Talos regression. I now have two of these to look
> into. I guess I'll need to look at discrete data to make any progress.

This could be due to an extra GC run.

With the patch a loop that triggers calls to JSContext::malloc but which does not allocate GC things would not trigger GC. The would be simply no code paths that the loop executes that would check the counter. With the patch to ensure that the GC happens even in such case I have added js_TriggerGC, http://hg.mozilla.org/tracemonkey/file/60ec3940a434/js/src/jscntxt.cpp#l1897 . With that call any looping code should eventually observe the request to GC and trigger that.

A simple way to check this is to comment out that line so only allocation of new GC things would notice the malloc pressure.
http://hg.mozilla.org/mozilla-central/rev/37cfb3befe00
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: