Closed Bug 677411 Opened 11 years ago Closed 10 years ago

GC: Refactor GC timers

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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)
(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 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)
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)
This patch adds some telemetry histograms for JS GC.
Attachment #561933 - Flags: review?(tglek)
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 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+
You need to log in before you can comment on or make changes to this bug.