GC: Refactor GC timers

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla10
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 551614 [details] [diff] [review]
patch

This patch rewrites the GCTimer code. My main goal was to have it permanently stored in the JSRuntime, which is needed for incremental GC. I also did some cleanups here and there.

The main change in functionality is that timers are now always compiled in. To enable the gcTimer.dat file, you have to turn on an environment variable.

Eventually, I would like to add some telemetry support. But that can be a separate bug.

Gregor, I suspect you use this code the most. But if you don't have time to review it, please assign it to Chris.
Attachment #551614 - Flags: review?(anygregor)
Comment on attachment 551614 [details] [diff] [review]
patch

I have to pass it on.
I like the fact that it is always on. Setting MOZ_GCTIMER to a filename gives the current behavior?
Attachment #551614 - Flags: review?(anygregor) → review?(cdleary)
(Assignee)

Comment 2

6 years ago
(In reply to Gregor Wagner from comment #1)
> I like the fact that it is always on. Setting MOZ_GCTIMER to a filename
> gives the current behavior?

Correct.
(Assignee)

Comment 3

6 years ago
Comment on attachment 551614 [details] [diff] [review]
patch

I realized I'd like to make some more changes. I'll repost when it's ready.
Attachment #551614 - Flags: review?(cdleary)
(Assignee)

Comment 4

6 years ago
Created attachment 561930 [details] [diff] [review]
updated gc stats patch

Here's an updated patch. I've tried to make it so that all GC instrumentation funnels through this code. This includes dtrace probes, crash data, and (in the next patch) telemetry.
Attachment #551614 - Attachment is obsolete: true
Attachment #561930 - Flags: review?(cdleary)
(Assignee)

Comment 5

6 years ago
Created attachment 561933 [details] [diff] [review]
GC telemetry patch

This patch adds some telemetry histograms for JS GC.
Attachment #561933 - Flags: review?(tglek)

Comment 6

6 years ago
Comment on attachment 561933 [details] [diff] [review]
GC telemetry patch

I think Luke is better suited for this particular uglyness.

Personally I would skip the switch statement and do
Telemetry::Accumulate(Telemetry::GC_REASON+id, sample);

I would even consider having a JSTelemetry.h somewhere in js/src included from Telemetry.h so we do not have to worry about histograms getting out of sync. I think privacy guys will be ok with having to check one extra file to avoid so much ugly.
Attachment #561933 - Flags: review?(tglek) → review?(luke)

Comment 7

6 years ago
Comment on attachment 561933 [details] [diff] [review]
GC telemetry patch

Can you put JS_SetAccumulateTelemetryCallback in jsfriendapi.h/cpp ?
Attachment #561933 - Flags: review?(luke) → review+
Comment on attachment 561930 [details] [diff] [review]
updated gc stats patch

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

Purdy. Only small nits.

::: js/src/gc/Statistics.cpp
@@ +13,5 @@
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is SpiderMonkey global object code.

Not global object code.

@@ +38,5 @@
> + * ***** END LICENSE BLOCK ***** */
> +
> +#include <stdio.h>
> +
> +#include "gc/Statistics.h"

Luke tells me that we put the directory-based ones below all the non-directory ones.

::: js/src/gc/Statistics.h
@@ +13,5 @@
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
> + * for the specific language governing rights and limitations under the
> + * License.
> + *
> + * The Original Code is SpiderMonkey global object code.

Ditto.

@@ +60,5 @@
> +    ALLOCTRIGGER,
> +    CHUNK,
> +    SHAPE,
> +    REFILL,
> +    NOREASON

Could we put a comment as to when we see/get NOREASON? Also maybe want to make this the zero enum value so you can say |if (!reason)| and |JS_ASSERT(reason)|?
Attachment #561930 - Flags: review?(cdleary) → review+
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dff0cfa7f23
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8b3a296e3f
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/9dff0cfa7f23
https://hg.mozilla.org/mozilla-central/rev/bf8b3a296e3f

Follow-up: https://hg.mozilla.org/mozilla-central/rev/0e4cee9a1e75
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.