587805 - TM: Clean GCMeter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gregor Wagner [:gwagner]]

There are members in JSGCStats that are never used.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

[Gregor Wagner [:gwagner]]

Or maybe we should discuss what we still want and add the functionality again.

Comment 3

[Brendan Eich [:brendan]]

Metering that's not enabled in DEBUG builds just bitrots. We are trying to keep useful instrumentation around, unrotted, by making it #ifdef DEBUG but enabling the costly parts depend on an envariable naming a file, to which to write logs or stats or dumps, being set. Search for getenv calls.

/be

Comment 4

[Gregor Wagner [:gwagner]]

GCMETER is defined in a debug build. Everything compiles fine. The problem is that we no longer record the data.

Comment 5

[Igor Bukanov]

Comment on attachment 466436 [details] [diff] [review]
patch

>-    METER_UPDATE_MAX(rt->gcStats.maxnchunks, rt->gcStats.nchunks);
...
>-    METER_UPDATE_MAX(rt->gcStats.maxnallarenas, rt->gcStats.nallarenas);

What is the reason for the removal of these two counters? They are definitely used and updated.

Comment 6

[Gregor Wagner [:gwagner]]

(In reply to comment #5)
> Comment on attachment 466436 [details] [diff] [review]
> patch
> 
> >-    METER_UPDATE_MAX(rt->gcStats.maxnchunks, rt->gcStats.nchunks);
> ...
> >-    METER_UPDATE_MAX(rt->gcStats.maxnallarenas, rt->gcStats.nallarenas);
> 
> What is the reason for the removal of these two counters? They are definitely
> used and updated.

Yes they are updated but where are they used?

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> > >-    METER_UPDATE_MAX(rt->gcStats.maxnchunks, rt->gcStats.nchunks);
> > >-    METER_UPDATE_MAX(rt->gcStats.maxnallarenas, rt->gcStats.nallarenas);
> > 
> > What is the reason for the removal of these two counters? They are definitely
> > used and updated.
> 
> Yes they are updated but where are they used?

Right, but this is a simple oversight (on my side!) not to include the counters in the stats printouts. So r+ with adding the counters to the printouts.

Comment 8

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 9

[Igor Bukanov]

Comment on attachment 466676 [details] [diff] [review]
patch

nice!

Comment 10

[Gregor Wagner [:gwagner]]

http://hg.mozilla.org/tracemonkey/rev/3ca837da1110

Comment 11

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3ca837da1110