677411 - GC: Refactor GC timers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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.

Comment 1

[Gregor Wagner [:gwagner]]

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?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: 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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: GC telemetry patch

This patch adds some telemetry histograms for JS GC.

Comment 6

[(dormant account)]

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.

Comment 7

[Luke Wagner [:luke]]

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

Can you put JS_SetAccumulateTelemetryCallback in jsfriendapi.h/cpp ?

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

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)|?

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dff0cfa7f23
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8b3a296e3f

Comment 10

[Ed Morley [:emorley]]

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