add API to enable/disable GCHelperThread

NEW
Assigned to

Status

()

Core
JavaScript Engine
7 years ago
3 years ago

People

(Reporter: gwagner, Assigned: gwagner)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

7 years ago
Assignee: general → anygregor
(Assignee)

Comment 2

7 years ago
Created attachment 538439 [details] [diff] [review]
patch

Add about:config option to disable parallel marking and background finalize.
Attachment #528901 - Attachment is obsolete: true
(Assignee)

Comment 3

7 years ago
Created attachment 538441 [details] [diff] [review]
patch

update
Attachment #538439 - Attachment is obsolete: true
(Assignee)

Comment 4

7 years ago
Created attachment 538447 [details] [diff] [review]
patch

update.
based on bug 638660.
Attachment #538441 - Attachment is obsolete: true
Attachment #538447 - Flags: review?(igor)
(Assignee)

Updated

7 years ago
Blocks: 638660
(Assignee)

Comment 5

7 years ago
Created attachment 538556 [details] [diff] [review]
patch

Fix Typo...
Attachment #538447 - Attachment is obsolete: true
Attachment #538447 - Flags: review?(igor)

Comment 6

7 years ago
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 &&
Attachment #538556 - Flags: review+
(Assignee)

Comment 7

7 years ago
Created attachment 539253 [details] [diff] [review]
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!
Attachment #538556 - Attachment is obsolete: true
Attachment #539253 - Flags: review?(igor)

Comment 8

7 years ago
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.
Attachment #539253 - Flags: review?(igor) → review+
(Assignee)

Comment 9

7 years ago
Created attachment 539292 [details] [diff] [review]
patch

addressing review comments.
This will land right after bug 638660.
Attachment #539253 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.