Last Comment Bug 677411 - GC: Refactor GC timers
: GC: Refactor GC timers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla10
Assigned To: Bill McCloskey (:billm)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-08 16:31 PDT by Bill McCloskey (:billm)
Modified: 2011-10-11 16:27 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (50.24 KB, patch)
2011-08-08 16:31 PDT, Bill McCloskey (:billm)
no flags Details | Diff | Splinter Review
updated gc stats patch (54.92 KB, patch)
2011-09-22 17:06 PDT, Bill McCloskey (:billm)
cdleary: review+
Details | Diff | Splinter Review
GC telemetry patch (9.48 KB, patch)
2011-09-22 17:09 PDT, Bill McCloskey (:billm)
luke: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2011-08-08 16:31:08 PDT
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.
Comment 1 Gregor Wagner [:gwagner] 2011-08-11 14:13:00 PDT
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 (:billm) 2011-08-11 20:16:16 PDT
(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 (:billm) 2011-08-22 10:22:24 PDT
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 (:billm) 2011-09-22 17:06:24 PDT
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.
Comment 5 Bill McCloskey (:billm) 2011-09-22 17:09:12 PDT
Created attachment 561933 [details] [diff] [review]
GC telemetry patch

This patch adds some telemetry histograms for JS GC.
Comment 6 (dormant account) 2011-09-22 17:23:29 PDT
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] 2011-09-22 21:47:02 PDT
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) 2011-09-26 13:51:11 PDT
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)|?

Note You need to log in before you can comment on or make changes to this bug.