653488 - add API to enable/disable GCHelperThread

Status: NEW

(Core :: JavaScript Engine, defect)

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

[Gregor Wagner [:gwagner]]

Attachment: patch

Add about:config option to disable parallel marking and background finalize.

Comment 3

[Gregor Wagner [:gwagner]]

Attachment: patch

update

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: patch

update.
based on bug 638660.

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: patch

Fix Typo...

Comment 6

[Igor Bukanov]

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

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

The patch should update the shell gcparam function, http://hg.mozilla.org/tracemonkey/file/3acacde59381/js/src/shell/js.cpp#l1531 with the new constants. r+ with that and comments below fixed.

::: js/src/jsapi.h
@@ +1815,5 @@
>  
>      /* Select GC mode. */
>      JSGC_MODE = 6,
> +    JSGC_BACKGROUND_FINALIZE = 7,
> +    JSGC_PARALLEL_MARKING = 8,

Nit: a blank line and comment before each constant.

::: js/src/jscntxt.h
@@ +425,5 @@
>      JSObject           *gcWeakMapList;
>  
> +    JSGCMode               gcMode;
> +    JSGCBackgroundFinalize gcBackgroundFinalize;
> +    JSGCParallelMarking    gcParallelMarking;

Define the flags like:

bool gcHasBackgroundFinalize;
bool gcHasParallelMarking;

and do the conversion from the enumeration type to the boolean in the API call. This way the flags usage in jsgc.cpp  would be less verbose and easier to follow.

::: js/src/jsgc.cpp
@@ +1246,5 @@
>       */
>      JS_ASSERT(backgroundFinalizeState == BFS_DONE ||
>                backgroundFinalizeState == BFS_JUST_FINISHED);
>  
> +    if (cx->runtime->gcBackgroundFinalize == JSGC_BACKGROUND_FINALIZE_ENABLED &&

With the type changes this line becomes:

if (cx->runtime->gcHasBackgroundFinalize &&

Comment 7

[Gregor Wagner [:gwagner]]

Attachment: patch

Yeah it's still pretty verbose but I didn't want to use short versions like BG for background because it's hard to read.
Thx!

Comment 8

[Igor Bukanov]

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

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

Few more comments:

::: js/src/jsapi.cpp
@@ +2662,5 @@
>          break;
>        case JSGC_STACKPOOL_LIFESPAN:
>          rt->gcEmptyArenaPoolLifespan = value;
>          break;
> +      case JSGC_BACKGROUND_FINALIZE:

Comment here that we do not disable/enable the background GC thread and for simplicity just set the flags that control how GC uses it.

After the bug 649537 we may need to reconsider this, but this is for another patch.

@@ +2664,5 @@
>          rt->gcEmptyArenaPoolLifespan = value;
>          break;
> +      case JSGC_BACKGROUND_FINALIZE:
> +        JS_ASSERT(JSGCBackgroundFinalize(value) == JSGC_BACKGROUND_FINALIZE_ENABLED ||
> +                  JSGCBackgroundFinalize(value) == JSGC_BACKGROUND_FINALIZE_DISABLED);

IIRC C/C++ allows for a compiler to assume that after JSGCBackgroundFinalize(value) the value fits the enum. So a smart compiler may eliminate the assert. So write here and similarly for another case:

JS_ASSERT(value == JSGC_BACKGROUND_FINALIZE_ENABLED ||
value == JSGC_BACKGROUND_FINALIZE_DISABLED);

@@ +2668,5 @@
> +                  JSGCBackgroundFinalize(value) == JSGC_BACKGROUND_FINALIZE_DISABLED);
> +        if (JSGCBackgroundFinalize(value) == JSGC_BACKGROUND_FINALIZE_ENABLED)
> +            rt->gcBackgroundFinalize = true;
> +        else
> +            rt->gcBackgroundFinalize = false;

Replace the "if" with:

rt->gcBackgroundFinalize = (value == JSGC_BACKGROUND_FINALIZE_ENABLED);

and similarly for another flag.

Comment 9

[Gregor Wagner [:gwagner]]

Attachment: patch

addressing review comments.
This will land right after bug 638660.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.