Checking for MaybeGC conditions when allocating GC things

RESOLVED WONTFIX

Status

()

enhancement
P2
normal
RESOLVED WONTFIX
11 years ago
10 years ago

People

(Reporter: igor, Assigned: andrei)

Tracking

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(17 obsolete attachments)

Reporter

Description

11 years ago
Currently the allocator for GC thing runs the last ditch only GC when the malloc limit is reached and when malloc returns NULL. In particular, there is no way to force the last-ditch GC when the garbage heuristic used by JS_MaybeGC in the browser or MaybeGC from the DOM is met.

This forces the application to worry about scheduling the GC from the application callback. Moreover, with the watchdog thread functionality from the bug 453157, this would force the watchdog thread to wake up more often the necessary just to check for MaybeGC conditions. 

Thus the idea is to allow for an embedding to specify that the GC thing allocator should run the last ditch GC when the MaybeGC conditions are met.

Updated

11 years ago
Summary: The last-ditch GC as maybe GC → Make it possible for MaybeGC to trigger a last-ditch GC
Assignee

Comment 1

11 years ago
Posted patch Work in progress (obsolete) — Splinter Review
Reporter

Comment 2

11 years ago
Comment on attachment 341919 [details] [diff] [review]
Work in progress

patch note: remember to pass -p option to the diff command when generating the patch so the diff would contain function names for the diff blocks. Here is non-nit comments:

>diff -r 1630d60e624e dom/src/base/nsJSEnvironment.cpp
...
>@@ -3419,16 +3388,17 @@
> //static
> PRBool
> nsJSContext::MaybeCC(PRBool aHigherProbability)
> {
>   ++sDelayedCCollectCount;
> 
>   // Don't check suspected count if CC will be called anyway.
>   if (sCCSuspectChanges <= NS_MIN_SUSPECT_CHANGES ||
>+      JS_GetGCParameter(nsJSRuntime::sRuntime, JSGC_NUMBER) -
>       sGCCount <= NS_MAX_GC_COUNT) {

Although this may not be observed in practice, the GC_NUMBER counter here may overflow uint32 when sGCCount was around max uin32. In this case the condition will true and trigger the cycle collector activities. This is harmless, but comments are due here.

>   if (!sGCTimer &&
>       (sDelayedCCollectCount > NS_MAX_DELAYED_CCOLLECT) &&
>       ((sCCSuspectChanges > NS_MIN_SUSPECT_CHANGES &&
>-        sGCCount > NS_MAX_GC_COUNT) ||
>+       JS_GetGCParameter(nsJSRuntime::sRuntime, JSGC_NUMBER)
>+       - sGCCount > NS_MAX_GC_COUNT) ||

Hm, this constructions JS_GetGCParameter(nsJSRuntime::sRuntime, JSGC_NUMBER) - sGCCount happens 3 times in the file. I suggest to have an inline function called like GetGCRunsCount and put the comments about overflow there. 

>diff -r 1630d60e624e js/src/js.cpp
>@@ -846,34 +846,45 @@
>     }
>     paramName = JS_GetStringBytes(str);
>     if (!paramName)
>         return JS_FALSE;
>     if (strcmp(paramName, "maxBytes") == 0) {
>         param = JSGC_MAX_BYTES;
>     } else if (strcmp(paramName, "maxMallocBytes") == 0) {
>         param = JSGC_MAX_MALLOC_BYTES;
>+    } else if (strcmp(paramName, "gcStackpoolLifespan") == 0) {
>+        param = JSGC_STACKPOOL_LIFESPAN;
>+    }else if (strcmp(paramName, "gcBytes") == 0) {
>+    	return JS_NewNumberValue(cx, JS_GetGCParameter(cx->runtime, JSGC_BYTES), &vp[0]);
>+    } else if (strcmp(paramName, "gcNumber") == 0) {
>+        return JS_NewNumberValue(cx, JS_GetGCParameter(cx->runtime, JSGC_NUMBER), &vp[0]);

The function should not simply return the value for read-only params. Rather it should report an error when it is called as a setter like in gcparam("gcNumber", 1)

> static JSBool
>diff -r 1630d60e624e js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp	Sun Oct 05 20:30:09 2008 -0700
>+++ b/js/src/jsapi.cpp	Mon Oct 06 16:36:13 2008 +0200
> 
> JS_PUBLIC_API(void)
>@@ -2577,16 +2579,42 @@
>         rt->gcMaxBytes = value;
>         break;
>       case JSGC_MAX_MALLOC_BYTES:
>         rt->gcMaxMallocBytes = value;
>         break;
>       case JSGC_STACKPOOL_LIFESPAN:
>         rt->gcEmptyArenaPoolLifespan = value;
>         break;
>+      case JSGC_FACTOR:
>+        rt->gcFactor = value;
>+        break;
>+      default:
>+        return;

The function should not be called for read-only parameters. So JS_NOT_REACHED("Attempt to mutate read-only parameter"); (a variation of JS_ASSERT(0) macro) should be before the return. In addition, the factor cannot be less then 100%, so the assignment to gcFactor should be protected with an assert.

>+    }
>+}
>+
>+JS_PUBLIC_API(uint32)
>+JS_GetGCParameter(JSRuntime *rt, JSGCParamKey key)
>+{
>+    switch (key) {
>+      case JSGC_MAX_BYTES:
>+        return rt->gcMaxBytes;
>+      case JSGC_MAX_MALLOC_BYTES:
>+        return rt->gcMaxMallocBytes;
>+      case JSGC_STACKPOOL_LIFESPAN:
>+        return rt->gcEmptyArenaPoolLifespan;
>+      case JSGC_FACTOR:
>+        return rt->gcFactor;
>+      case JSGC_BYTES:
>+        return rt->gcBytes;
>+      case JSGC_NUMBER:
>+        return rt->gcNumber;
>+      default:
>+        return -1;

It should be a bug to call the function with an unknown value of the key. So write the last case and the default as single default:
       
>+      default:
>+        JS_ASSERT(key == JSGC_NUMBER);
>+        return rt->gcNumber;

>diff -r 1630d60e624e js/src/jsapi.h
>--- a/js/src/jsapi.h	Sun Oct 05 20:30:09 2008 -0700
>+++ b/js/src/jsapi.h	Mon Oct 06 16:36:13 2008 +0200
>@@ -1125,21 +1125,33 @@
> typedef enum JSGCParamKey {
>     /* Maximum nominal heap before last ditch GC. */
>     JSGC_MAX_BYTES          = 0,
> 
>     /* Number of JS_malloc bytes before last ditch GC. */
>     JSGC_MAX_MALLOC_BYTES   = 1,
> 
>     /* Hoard stackPools for this long, in ms, default is 30 seconds. */
>-    JSGC_STACKPOOL_LIFESPAN = 2
>+    JSGC_STACKPOOL_LIFESPAN = 2,
>+
>+    /* Factor that defines when GC starts to work. */
>+    JSGC_FACTOR = 3,

This requires detailed comments especially given the percentage nature of the factor. Also, just "factor" is too generic. Something more descriptive like JSGC_THRESHOLD_FACTOR or JSGC_TRIGGER_FACTOR or JSGC_???_FASCTOR would be more appropriate. 

>diff -r 1630d60e624e js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp	Sun Oct 05 20:30:09 2008 -0700
>+++ b/js/src/jsgc.cpp	Mon Oct 06 16:36:13 2008 +0200
>@@ -1819,16 +1819,17 @@
>     JS_ASSERT(!rt->gcRunning);
>     if (rt->gcRunning) {
>         METER(rt->gcStats.finalfail++);
>         JS_UNLOCK_GC(rt);
>         return NULL;
>     }
> 
>     doGC = (rt->gcMallocBytes >= rt->gcMaxMallocBytes && rt->gcPoke);
>+    doGC = doGC || (rt->gcBytes > 8192 && rt->gcBytes > rt->gcLastBytes * rt->gcFactor / 100);

This can overflow. One way to workaround this while keeping enough precision is to write it as:

  rt->gcBytes / rt->gcFactor > rt->gcLastBytes / 100 

Plus this condition, rt->gcBytes > 8192 && rt->gcBytes > rt->gcLastBytes * rt->gcFactor / 100, should really be moved to an inline function, something like IsReachedGCThreshold(JSRuntime *rt).

>+        || (rt->gcBytes > 8192 &&
>+           (uint32)((uint64)rt->gcBytes * 100 / rt->gcLastBytes) >
>+            rt->gcFactor)) {
>         goto do_gc;

Here the condition is checked the second times, but the changes has also be added to js_AddAsGCBytes.
Assignee

Comment 3

11 years ago
Posted patch Fix (obsolete) — Splinter Review
Attachment #341919 - Attachment is obsolete: true
Attachment #342094 - Flags: review?(igor)
Reporter

Comment 4

11 years ago
Changing the title to better reflect the nature of the bug.
Summary: Make it possible for MaybeGC to trigger a last-ditch GC → Checking for MaybeGC conditions when allocating GC things
Reporter

Comment 5

11 years ago
Comment on attachment 342094 [details] [diff] [review]
Fix

General question: I do not see the trigger factor is ever set for the browser. This should be done in nsXPCGlobals::EnsureJSRuntime from js/src/xpconnect/src/nsXPCGlobals.cpp at the same place where it sets JSGC_MAX_BYTES. That would also require to update the comments there.

>diff -r 197f83ad7678 dom/src/base/nsJSEnvironment.cpp
...
>@@ -884,18 +868,16 @@ nsJSContext::DOMOperationCallback(JSCont
>   }
> 
>   // XXX Save the operation callback time so we can restore it after the GC,
>   // because GCing can cause JS to run on our context, causing our
>   // ScriptEvaluated to be called, and clearing our operation callback time.
>   // See bug 302333.
>   PRTime callbackTime = ctx->mOperationCallbackTime;
> 
>-  MaybeGC(cx);
>-
>   // Now restore the callback time and count, in case they got reset.
>   ctx->mOperationCallbackTime = callbackTime;

The comments together with saving and restoration of mOperationCallbackTime should be removed. The GC is not called here!

>@@ -3399,37 +3368,49 @@ nsJSContext::CC()
...
>+inline uint32
>+GetGCRunsCount()

That should be static inline, not just inline. The static emphasis that this is file-scope helper and ensures that no symbols would be defined in the object files. 

>diff -r 197f83ad7678 js/src/js.cpp
...
>@@ -846,34 +846,58 @@ GCParameter(JSContext *cx, uintN argc, j
>     }
>     paramName = JS_GetStringBytes(str);
>     if (!paramName)
>         return JS_FALSE;
>     if (strcmp(paramName, "maxBytes") == 0) {
>         param = JSGC_MAX_BYTES;
>     } else if (strcmp(paramName, "maxMallocBytes") == 0) {
>         param = JSGC_MAX_MALLOC_BYTES;
>+    } else if (strcmp(paramName, "gcStackpoolLifespan") == 0) {
>+        param = JSGC_STACKPOOL_LIFESPAN;
>+    }else if (strcmp(paramName, "gcBytes") == 0) {

Nit: blank after }.

>+        if (argc > 1) {
>+            JS_ReportError(cx, "Attempt to change read-only parameter");
>+            return JS_FALSE;
>+        }

Nit: replace the word "parameter" with its name, "gcBytes", for better error reporting.

>+    	return JS_NewNumberValue(cx,
>+                                 JS_GetGCParameter(cx->runtime, JSGC_BYTES),
>+                                 &vp[0]);

Nit: bad indent. JS_GetGCParameter should start at the same column as cx to lineup everything under the parenthesis.

>+    } else if (strcmp(paramName, "gcNumber") == 0) {
>+        if (argc > 1) {
>+            JS_ReportError(cx, "Attempt to change read-only parameter");
>+            return JS_FALSE;
>+        }

Again, use "gcNumber", not "parameter".

>+        return JS_NewNumberValue(cx,
>+                                 JS_GetGCParameter(cx->runtime, JSGC_NUMBER),
>+                                 &vp[0]);

Here the indentation is right one :)

>+    } else if (strcmp(paramName, "gcTriggerFactor") == 0) {
>+        param = JSGC_TRIGGER_FACTOR;
>     } else {
>         JS_ReportError(cx,
>-                       "the first argument argument must be either maxBytes "
>-                       "or maxMallocBytes");
>-        return JS_FALSE;
>-    }
>-
>-    if (!JS_ValueToECMAUint32(cx, argc < 2 ? JSVAL_VOID : vp[3], &value))
>-        return JS_FALSE;
>-    if (value == 0) {
>-        JS_ReportError(cx,
>-                       "the second argument must be convertable to uint32 with "
>-                       "non-zero value");
>-        return JS_FALSE;
>-    }
>-    JS_SetGCParameter(cx->runtime, param, value);
>-    *vp = JSVAL_VOID;
>-    return JS_TRUE;
>+                       "the first argument argument must be maxBytes, "
>+                       "maxMallocBytes, gcStackpoolLifespan, gcBytes, "
>+                       "gcNumber, or gcTriggerFactor");

Nit: remove the comma before "or".

>+        return JS_FALSE;
>+    }
>+
>+    if (argc == 1) {
>+        return JS_NewNumberValue(cx, JS_GetGCParameter(cx->runtime, param), &vp[0]);
>+    } else {

Nit: remove the else as the "then" part of the if ends with return. In general, the style dictates to omit else whenever the "then" part ends with a control transfer. That is, do NOT use:

if (foo) {
    ...
    return/break/continue/goto
} else {
    ...
}

But rather write:

if (foo) {
    ...
    return/break/continue/goto
}
...

Also, after removal of the else part do not forget to remove the braces around the "then" part as it fits one line. 

>+        if (!JS_ValueToECMAUint32(cx, vp[3], &value)) {
>+            JS_ReportError(cx,
>+                           "the second argument must be convertable to uint32 with "
>+                           "non-zero value");
>+            return JS_FALSE;
>+        }

>+        JS_SetGCParameter(cx->runtime, param, value);

For JSGC_TRIGGER_FACTOR parameter the code should report an error when the value is below 100. 

>diff -r 197f83ad7678 js/src/jsapi.cpp
> 
>+    rt->gcTriggerFactor = (uint32)-1;

Nit: add a blank after (uint32).

>@@ -2580,16 +2582,50 @@ JS_SetGCParameter(JSRuntime *rt, JSGCPar
>         rt->gcMaxBytes = value;
>         break;
>       case JSGC_MAX_MALLOC_BYTES:
>         rt->gcMaxMallocBytes = value;
>         break;
>       case JSGC_STACKPOOL_LIFESPAN:
>         rt->gcEmptyArenaPoolLifespan = value;
>         break;
>+      case JSGC_TRIGGER_FACTOR:
>+        if (value < 100)
>+            JS_NOT_REACHED("Value of the trigger factor must be >= 100");

I was not clear in the prev comments: for the assert just write:

   JS_ASSERT(value >= 100);

JS_NOT_REACHED is for places where code is really unreached.

>+        rt->gcTriggerFactor = value;
>+        break;
>+      case JSGC_BYTES:
>+        JS_NOT_REACHED("Attempt to mutate read-only parameter");
>+        break;
>+      case JSGC_NUMBER:
>+        JS_NOT_REACHED("Attempt to mutate read-only parameter");
>+        break;
>+      default:
>+        JS_NOT_REACHED("Attempt to mutate unknown parameter");
>+        return;

Remove here the JSGC_BYTES and JSGC_NUMBER cases: the default is enough.


>diff -r 197f83ad7678 js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp	Tue Oct 07 11:45:10 2008 -0400
>+++ b/js/src/jsgc.cpp	Tue Oct 07 18:53:06 2008 +0200
>@@ -1752,16 +1752,23 @@ EnsureLocalFreeList(JSContext *cx)
>             return NULL;
>     }
>     cx->gcLocalFreeLists = freeLists;
>     return freeLists;
> }
> 
> #endif
> 
>+inline JSBool
>+IsGCThresholdReached(JSRuntime *rt)

Use here static JS_INLINE to replace just "inline".

>@@ -1819,16 +1826,17 @@ js_NewGCThing(JSContext *cx, uintN flags
..
>     doGC = (rt->gcMallocBytes >= rt->gcMaxMallocBytes && rt->gcPoke);
>+    doGC = doGC || IsGCThresholdReached(rt);

Nit: write the whole condition as

      doGC = (rt->gcMallocBytes >= rt->gcMaxMallocBytes && rt->gcPoke) ||
             IsGCThresholdReached(rt);

> 
>     if (rt->gcMallocBytes >= rt->gcMaxMallocBytes && rt->gcPoke
> #ifdef JS_GC_ZEAL
>         && (rt->gcZeal >= 2 || (rt->gcZeal >= 1 && rt->gcPoke))
> #endif

And here comes my own bug: the condition should be 
>     if (rt->gcMallocBytes >= rt->gcMaxMallocBytes && rt->gcPoke
> #ifdef JS_GC_ZEAL
>         || (rt->gcZeal >= 2 || (rt->gcZeal >= 1 && rt->gcPoke))
> #endif

Please fix this as well and write the whole thing as:

>     if ((rt->gcMallocBytes >= rt->gcMaxMallocBytes && rt->gcPoke) ||
          IsGCThresholdReached(rt)
> #ifdef JS_GC_ZEAL
>         || (rt->gcZeal >= 2 || (rt->gcZeal >= 1 && rt->gcPoke))
> #endif

>@@ -2206,17 +2214,17 @@ js_AddAsGCBytes(JSContext *cx, size_t sz
>     JSRuntime *rt;
> 
>     rt = cx->runtime;
>     if (rt->gcBytes >= rt->gcMaxBytes ||
>         sz > (size_t) (rt->gcMaxBytes - rt->gcBytes)
> #ifdef JS_GC_ZEAL
>         || rt->gcZeal >= 2 || (rt->gcZeal >= 1 && rt->gcPoke)
> #endif
>-        ) {
>+        || IsGCThresholdReached(rt)) {

Nit: move checking for IsGCThresholdReached(rt) before the zeal macro.
Attachment #342094 - Flags: review?(igor)
Reporter

Comment 6

11 years ago
Comment on attachment 342094 [details] [diff] [review]
Fix

>diff -r 197f83ad7678 js/src/jsapi.h
>@@ -1125,21 +1125,33 @@ typedef enum JSGCParamKey {
...
>+
>+    /* Factor that defines when GC starts to work. */
>+    JSGC_TRIGGER_FACTOR = 3,

One more thing: explain the nature of the factor in the comments and emphasis that it gives the number in percents and that it should be 100 or more.
Assignee

Comment 7

11 years ago
Posted patch Updated fix (obsolete) — Splinter Review
Attachment #342094 - Attachment is obsolete: true
Attachment #342257 - Flags: review?(igor)
Reporter

Comment 8

11 years ago
Comment on attachment 342257 [details] [diff] [review]
Updated fix

>diff -r 2bf271f5e732 dom/src/base/nsJSEnvironment.cpp
>--- a/dom/src/base/nsJSEnvironment.cpp	Wed Oct 08 14:35:59 2008 +0200
>+++ b/dom/src/base/nsJSEnvironment.cpp	Wed Oct 08 18:21:38 2008 +0200

> // The accumulated operation weight before we call MaybeGC
> const PRUint32 MAYBE_GC_OPERATION_WEIGHT = 5000 * JS_OPERATION_WEIGHT_BASE;

Nit: rename MAYBE_GC_OPERATION_WEIGHT into DOM_CALLBACK_OPERATION_WEIGHT as with the patch this has nothing to do with the GC.

>+static JS_INLINE uint32
>+GetGCRunsCount()

Nit: use inline here, not JS_INLINE, as the latter is just to improve compatibility of sources in js/src directory.

Also, the patch does not remove the calls to JS_MaybeGC from js.cpp. It should.

>diff -r 2bf271f5e732 js/src/js.cpp
>diff -r 2bf271f5e732 js/src/jsapi.cpp
> 
>+    /*
>+     * By default the trigger factor gets maximum possible value. This
>+     * means that GC will not be triggered by growth of GC memory (gcBytes).
>+     */
>+    rt->gcTriggerFactor = (uint32) -1;
>+    /*
>+     * The assigned value prevents GC from running when GC memory is too low
>+     * (during JS engine start).
>+     */

Nit: blank line between gcTriggerFactor assignment and comments. 

>+    rt->gcLastBytes = 8192;
>+

The bigger nit that I forgot in the previous comments: move these 2 assignments to js_InitGC function in jsgc.cpp. JS_NewRutime calls that function.

> JS_PUBLIC_API(void)
>@@ -2580,16 +2591,43 @@ JS_SetGCParameter(JSRuntime *rt, JSGCPar
>         rt->gcMaxBytes = value;
>         break;
>       case JSGC_MAX_MALLOC_BYTES:
>         rt->gcMaxMallocBytes = value;
>         break;
>       case JSGC_STACKPOOL_LIFESPAN:
>         rt->gcEmptyArenaPoolLifespan = value;
>         break;
>+      case JSGC_TRIGGER_FACTOR:
>+        JS_ASSERT(value >= 100);
>+        rt->gcTriggerFactor = value;
>+        break;
>+      default:
>+        JS_NOT_REACHED("Attempt to mutate read-only or unknown parameter");
>+        return;
>+    }
>

Nit (I missed this in today's discussion): remove the default: here altogether and replace case JSGC_TRIGGER_FACTOR: with default and an extra assert that the key is JSGC_TRIGGER_FACTOR in the same way as it is done in JS_GetGCParameter.

>diff -r 2bf271f5e732 js/src/jsgc.cpp
> 
>+static JS_INLINE JSBool
>+IsGCThresholdReached(JSRuntime *rt)
>+{
>+   return rt->gcBytes / rt->gcTriggerFactor >= rt->gcLastBytes / 100;
>+}

A comment is due in the function that explains why this condition will not be triggered on the start up. The comment should also refer to js_InitGC (where gcLastBytes will be set). 

>diff -r 2bf271f5e732 js/src/xpconnect/src/xpcruntimesvc.cpp
>@@ -239,63 +239,65 @@ NS_IMETHODIMP
> NS_IMETHODIMP
> nsJSRuntimeServiceImpl::GetRuntime(JSRuntime **runtime)
>         // Unconstrain the runtime's threshold on nominal heap size, to avoid
>         // triggering GC too often if operating continuously near an arbitrary
>         // finite threshold (0xffffffff is infinity for uint32 parameters).
>         // This leaves the maximum-JS_malloc-bytes threshold still in effect
>         // to cause period, and we hope hygienic, last-ditch GCs from within
>         // the GC's allocator.
>         JS_SetGCParameter(mRuntime, JSGC_MAX_BYTES, 0xffffffff);
>+        /* GC will be called when gcBytes is 1600% of gcLastBytes. */
>+        JS_SetGCParameter(mRuntime, JSGC_TRIGGER_FACTOR, 1600);


Nits: a blank line before the comments and use //, not /*, for commenting to follow the existing style.
Attachment #342257 - Flags: review?(igor)
Assignee

Comment 9

11 years ago
Posted patch Updated fix (obsolete) — Splinter Review
Attachment #342257 - Attachment is obsolete: true
Attachment #342348 - Flags: review?(igor)
Reporter

Comment 10

11 years ago
Comment on attachment 342348 [details] [diff] [review]
Updated fix

>diff -r 2bf271f5e732 js/src/js.cpp
>@@ -180,24 +180,19 @@ my_BranchCallback(JSContext *cx, JSScrip
...
> #ifdef JS_THREADSAFE
>-    if ((gBranchCount & 0xff) == 1) {
>-#endif
>-        if ((gBranchCount & 0x3fff) == 1)
>-            JS_MaybeGC(cx);
>-#ifdef JS_THREADSAFE
>-        else
>-            JS_YieldRequest(cx);
>-    }
>+    if ((gBranchCount & 0xff) == 1 &&
>+        (gBranchCount & 0x3fff) != 1)
>+        JS_YieldRequest(cx);
> #endif

This can be simplified to just  

> #ifdef JS_THREADSAFE
>+    if ((gBranchCount & 0xff) == 1)
>+        JS_YieldRequest(cx);
> #endif

since such change would change the frequency of JS_YieldRequest invocations by factor of 1 + 1/0x3f or 1 + 1/63 or 1.016 and such change can be ignored.
Attachment #342348 - Flags: review?(igor)
Assignee

Comment 11

11 years ago
Posted patch Updated fix (obsolete) — Splinter Review
Attachment #342348 - Attachment is obsolete: true
Attachment #342425 - Flags: review?(brendan)
Assignee

Comment 12

11 years ago
Assignee

Comment 13

11 years ago
Attachment #342425 - Attachment is obsolete: true
Attachment #343083 - Flags: review?(brendan)
Attachment #342425 - Flags: review?(brendan)
Assignee

Comment 14

11 years ago
Attachment #342426 - Attachment is obsolete: true

Comment 15

11 years ago
I have a question. I met a build break when I tried to build firefox with this patch. It seems to me mRuntime argument need to be replaced with mJSRuntime.

+        // GC will be called when gcBytes is 1600% of gcLastBytes.
+        JS_SetGCParameter(mRuntime, JSGC_TRIGGER_FACTOR, 1600);
Reporter

Updated

11 years ago
Assignee: igor → andrei
Assignee

Comment 16

11 years ago
Posted patch Fix (obsolete) — Splinter Review
Compilation problem is fixed
Attachment #343083 - Attachment is obsolete: true
Attachment #344275 - Flags: review?(brendan)
Attachment #343083 - Flags: review?(brendan)
Assignee

Comment 17

11 years ago
Compilation problem is fixed
Attachment #343084 - Attachment is obsolete: true
Blocks: 456721
Reporter

Comment 18

11 years ago
To Brendan: do you have any chance to review this? 

This patch allows to decrease the frequency of the operation callback calls. That in turn together with the watchdog thread work from the bug 453157 will allow to eventually remove all high-frequency callback calls so the only purpose of the callback would be to show the slow script warning or to force DOM thread worker to leave the request so the GC can proceed.
Reporter

Comment 19

11 years ago
To Andrei: could you update the patch for the tip and ask Blake, mrbkap@gmail.com, for a review?
Sorry, I've been swamped -- I don't know if Blake has more time, but I will take a look after some deadlines. This bug is not a 3.1b2 blocker. Is it needed for 3.1 at all?

/be
Reporter

Comment 21

11 years ago
(In reply to comment #20)
> Sorry, I've been swamped -- I don't know if Blake has more time, but I will
> take a look after some deadlines. This bug is not a 3.1b2 blocker. Is it needed
> for 3.1 at all?

Yes, at least for the optimal performance of DOM workers and to address the bug 456721.

Updated

11 years ago
Flags: blocking1.9.1+
Assignee

Comment 22

11 years ago
Posted patch Updated MaybeGC patch (obsolete) — Splinter Review
Attachment #344275 - Attachment is obsolete: true
Attachment #347764 - Flags: review?(brendan)
Attachment #344275 - Flags: review?(brendan)
Assignee

Comment 23

11 years ago
Attachment #344276 - Attachment is obsolete: true
Comment on attachment 347764 [details] [diff] [review]
Updated MaybeGC patch

I'm still fighting deadlines -- Blake, if you have time feel free to take single r?. Thanks,

/be
Attachment #347764 - Flags: review?(mrbkap)
Assignee

Comment 25

11 years ago
Blake, could you review this patch ASAP? It can cause some problems as the watchdog patch (https://bugzilla.mozilla.org/show_bug.cgi?id=453157) did, since both are related to the GC timing. So the earlier we start to check in and test the earlier we can find and fix possible problems.
Comment on attachment 347764 [details] [diff] [review]
Updated MaybeGC patch

>diff -r 3eaa593394b4 js/src/js.cpp
>@@ -861,34 +855,64 @@ GCParameter(JSContext *cx, uintN argc, j
>     }
>     paramName = JS_GetStringBytes(str);
>     if (!paramName)
>         return JS_FALSE;
>     if (strcmp(paramName, "maxBytes") == 0) {
>         param = JSGC_MAX_BYTES;
>     } else if (strcmp(paramName, "maxMallocBytes") == 0) {
>         param = JSGC_MAX_MALLOC_BYTES;
>+    } else if (strcmp(paramName, "gcStackpoolLifespan") == 0) {
>+        param = JSGC_STACKPOOL_LIFESPAN;
>+    } else if (strcmp(paramName, "gcBytes") == 0) {
>+        if (argc > 1) {
>+            JS_ReportError(cx,
>+                           "Attempt to change read-only parameter gcBytes");
>+            return JS_FALSE;
>+        }
>+        return JS_NewNumberValue(cx,
>+                                 JS_GetGCParameter(cx->runtime, JSGC_BYTES),
>+                                 &vp[0]);
>+    } else if (strcmp(paramName, "gcNumber") == 0) {
>+        if (argc > 1) {
>+            JS_ReportError(cx,
>+                           "Attempt to change read-only parameter gcNumber");
>+            return JS_FALSE;
>+        }
>+        return JS_NewNumberValue(cx,
>+                                 JS_GetGCParameter(cx->runtime, JSGC_NUMBER),
>+                                 &vp[0]);
>+    } else if (strcmp(paramName, "gcTriggerFactor") == 0) {
>+        param = JSGC_TRIGGER_FACTOR;
>     } else {
>         JS_ReportError(cx,
>-                       "the first argument argument must be either maxBytes "
>-                       "or maxMallocBytes");
>-        return JS_FALSE;
>-    }

Nit: SpiderMonkey avoids else-after-return. However, I don't see why you have the early returns at all. It seems like you can enforce argc == 1, set param = the right JSGC_* parameter and fall into the argc == 1 case below. r=mrbkap with that.
Attachment #347764 - Flags: review?(mrbkap) → review+
Assignee

Comment 27

11 years ago
Posted patch Updated patch (obsolete) — Splinter Review
Patch reviewed by Blake and updated according to his suggestions.
Attachment #347764 - Attachment is obsolete: true
Attachment #347765 - Attachment is obsolete: true
Attachment #350171 - Flags: review+
Attachment #347764 - Flags: review?(brendan)
Assignee

Comment 28

11 years ago
Posted file The changeset created by hg bundle (obsolete) —
I tried checking it in, but it caused unit test orange.  In particular, the test_host.js test in netwerk/test/httpserver failed.

The failure was locally reproducible over here by running:

  make -C ../obj-firefox-opt/netwerk/test/httpserver/ check

with this patch in my tree.  Without this patch, the test passed.  There were also mailnews test failures on the Thunderbird tinderbox.

I backed the patch out; please make sure to run unit tests on the next revision?
Assignee

Comment 30

11 years ago
Boris, could you specify more information about your tests? I tried to reproduce the test failure and I could not do this on my platform x64 Linux. I actually found when the tests failed. But this happened when I built with --enable-trace-malloc. And it failed with or without the patch.

Updated

11 years ago
Priority: -- → P1

Comment 31

11 years ago
When I ran the unit test with this patch, I didn’t meet the test failure as well as Andrei. 
My test result is as below,

    TEST-PASS | ../../../_tests/xpcshell-simple/test_necko/test/test_host.js | all tests passed

Does the test failure still occur on this patch?

If I did some mistakes in my unit test, please let me know. Thank you.

System Under Test
 - OS : Linux (Ubuntu 8.04)
 - CPU :  Intel(R) Core(TM)2 Duo CPU E7300  @ 2.66GHz

 - RAM : 2GB
 - Source code : mozilla-central on 2008.01.09 with this patch
Reporter

Comment 32

11 years ago
(In reply to comment #31)
> When I ran the unit test with this patch, I didn’t meet the test failure as
> well as Andrei. 

The tests failed on Mac, on Linux there were no problems.
Assignee

Comment 33

11 years ago
The tests that fails is identified. It fails both on Mac and Linux.
If fails while sending 2^14 blank lines. 
_tests/xpcshell-simple/test_necko/test/test_request_line_split_in_two_packets.js
According to the existing logs it looks like client input stream (when it reads response) returns empty string while the http server continues to process blank lines. 
I continue to search for the problem.
Assignee

Comment 34

11 years ago
It was discovered that the test failure was caused by GC run (see bug 474312). So the patch was updated to avoid the GC run during the xpcshell tests.
Attachment #350171 - Attachment is obsolete: true
Attachment #350172 - Attachment is obsolete: true
Attachment #357739 - Flags: review?(igor)
Reporter

Comment 35

11 years ago
Comment on attachment 357739 [details] [diff] [review]
Patch with workaround for xcpshell tests

>diff -r 3eb79df3bcb0 dom/src/base/nsJSEnvironment.cpp
...
>@@ -3307,31 +3281,17 @@ nsJSContext::ScriptEvaluated(PRBool aTer
>-  mNumEvaluations++;
>-
>-#ifdef JS_GC_ZEAL
>-  if (mContext->runtime->gcZeal >= 2) {
>-    MaybeGC(mContext);
>-  } else
>-#endif
>-  if (mNumEvaluations > 20) {
>-    mNumEvaluations = 0;
>-    MaybeGC(mContext);
>-  }
>-
>-  if (aTerminated) {
>-    mOperationCallbackTime = LL_ZERO;
>-  }
>+  mOperationCallbackTime = LL_ZERO;
> }

This is a bad merge (added in some earlier version of the patch) - the patch should not touch the aTerminated check introduced in the bug 459653, http://hg.mozilla.org/mozilla-central/rev/995c50549d17. Otherwise the patch is OK.
Attachment #357739 - Flags: review?(igor) → review-
Assignee

Comment 36

11 years ago
Attachment #357739 - Attachment is obsolete: true
Attachment #357778 - Flags: review?(igor)
Reporter

Updated

11 years ago
Attachment #357778 - Flags: review?(igor) → review+
Reporter

Comment 37

11 years ago
landed to TM - http://hg.mozilla.org/tracemonkey/rev/e74857ea8248
Whiteboard: fixed-in-tracemonkey
Reporter

Comment 38

11 years ago
this caused failures on Mac tinderbox in TUnit tests - http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1232463374.1232466190.24202.gz , so backing out.
Reporter

Comment 39

11 years ago
On Tinderbox the Linux unit tests have also showed the same failures with the patch from the comment 20 as on Mac's tinderbox, http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1232463374.1232469745.29730.gz.
Assignee

Comment 40

11 years ago
Attachment #357778 - Attachment is obsolete: true
Attachment #357955 - Flags: review?(igor)
Reporter

Updated

11 years ago
Attachment #357955 - Flags: review?(igor) → review+
Assignee

Updated

11 years ago
Depends on: 474801
Igor, Andrei, Since this bug is blocking bug 468840 owned by dmandelin, the patches here reportedly conflict in a bad way with dmandelin's patch, and it's also a P1 bug, can you guys make sure to be extra communicative in this bug and with dmandelin directly via IRC so that he can work in parallel with you guys to redo his patch so there's no conflict?
Reporter

Updated

11 years ago
Depends on: 475680
Reporter

Updated

10 years ago
Attachment #357955 - Attachment is obsolete: true
Reporter

Comment 44

10 years ago
The current approach for this bug does not work since the last ditch GC never collects property ids and other atoms. It is possible to fix this via forcing an operation callback call and then doing the requested full GC from that place, but it complicate things.

Moreover, the suggested heuristics for the bug 468840 makes checking for the GC conditions rather complex affair which is not suitable for doing when allocating each and every GC page. Thus this bug would not eliminate the need to call MaybeGC periodically.

Thus I suggest to either mark this bug as wontfix or at least remove P1 and 1.9.1 blocker flags from it.

Comment 45

10 years ago
Marking down for now, dmandelin and brendan should give their opinion on WONTFIX (sounds reasonable to me).
Priority: P1 → P2
(In reply to comment #45)
> Marking down for now, dmandelin and brendan should give their opinion on
> WONTFIX (sounds reasonable to me).

Sounds good to me. It looks like the issues are just too complicated to sort out in time for b3.

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.