Closed Bug 665247 Opened 13 years ago Closed 13 years ago

can we throw away a lot of this #ifdef'd metering code?

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

First, to be clear, I'm referring to the metering code that isn't on in release builds, so not the stuff that contributes to about:memory.  The purpose of the metering code in theory is to provide ready-to-use stats so that we can use them when the need arises.  However, when I measure something, I tend to find the particular cases I care about, stick my own custom instrumentation in those points (maybe its rdtsc, maybe its a counter, maybe some crazy hash table), get in, get the data, get out.  I never use the pre-existing instrumentation because (1) it takes time to see if its measuring what I want to measure (2) b/c its usually off, its probably wrong so I'm going to have to audit it so I might as well just write my own code.

I've talked to several other developers who feel more or less the same way.
There is a non-trivial always-on overhead to this metering code too: when I change code, I waste brain cycles trying to preserve existing metering behavior.  When I read code, the SHOUTY_MACROS clutter my screen.

So, can we declare it open season on metering?  Perhaps there are one or two instances which are commonly used?  Perhaps these could be maintained in separate repos (like my "measure" repo)?  Discussion welcome.
Summary: can we throw away a lot of these #ifdef'd metering code? → can we throw away a lot of this #ifdef'd metering code?
(In reply to comment #0)
> So, can we declare it open season on metering?  Perhaps there are one or two
> instances which are commonly used?  Perhaps these could be maintained in
> separate repos (like my "measure" repo)?  Discussion welcome.

+1.

- Metering code isn't part of standard builds, so it tends to be broken when you turn it on.

- Most of these do end up as one-offs in retrospect. We might want to keep 1 or 2 high-value ones, but I'm inclined to say pull them out into patches, keep the patch, if someone ever needs it they can rebase and reapply.

- I figure that if there is a metering patch that is used regularly enough that we want it, we'll know, and we can turn it into a proper tool that is a standard part of debug builds.
"Me too!"

Luke, can you list the #ifdefs that would be removed?

IIRC Brendan has some code for generating histograms from counts of events.  That kind of utility might be useful to keep in debug builds, because it might make ad hoc instrumentation easier.
So I looked around and I had no idea how much metering code we have.  In particular, there seem to be two pretty big cases:
 - GC
 - property tree / shape
For these, I pose the question to Bill/Igor/Gregor/Jason: is it worth it to leave the metering code in the tree rather than in a separate patch/repo?  I'm going to ignore the property table metering since its not in such active development.

There is also the JS_OPMETER and JS_REPRMETER.  I know REPRMETER was used in initial JM1 planning but I think we have moved pretty far past it by now, so do you think we can remove this Dave?  I've never used JS_OPMETER, but it does tend to waste my time when I refactor and end up in the code via vimgrep, so I'd like to pull that out into a patch too.

I think what comment 2 is referring to is the JSBasicStats functionality in jsutil.h/.cpp.  This seems pretty reusable and its out of the way, so it would make sense to keep it in the tree.

The main thing I filed the bug to remove is JS_RUNTIME_METER, JS_FUNCTION_METER and the JSRuntime members that are manipulated via these macros.  There is also some random dead code that uses JS_LOCK_RUNTIME_VOID.
I'll speak only for myself here, since I'm guessing everyone using this stuff differently.

I have used the shape dumping code once or twice, mostly just to get a sense of all the things that shapes are used for. This code is pretty easy to maintain, so I think it makes sense to keep it.

The ones that I really hate are the counters: things like livePropTreeNodes, liveDictModeNodes, and pretty much everything in js_scope_stats. I've never used this code, and I have no idea if it's correct or not. Some of these counts would probably be more reliably measured by dumping out the shape tree and then grepping the output. Others, like tableAllocFails, just seem totally useless to me.

There's also code that seems to keep a histogram of the number of children a shape has. This seems like a very specific use that would be better off as a separate utility that processes the output of dumping the shape tree. I'm all for getting rid of it. Then, if we need it again, we can write that utility.

I've used the GC timer code a lot, but never the GC metering code. I'm guessing Gregor is the main user of it.
(In reply to comment #3)
> There is also the JS_OPMETER and JS_REPRMETER.  I know REPRMETER was used in
> initial JM1 planning but I think we have moved pretty far past it by now, so
> do you think we can remove this Dave?

Yes.

> I've never used JS_OPMETER, but it
> does tend to waste my time when I refactor and end up in the code via
> vimgrep, so I'd like to pull that out into a patch too.

Yes again. Also, I think the data JS_OPMETER provides can be extracted from the output of Steve's runmode profiler now.
(In reply to comment #4)
> 
> I've used the GC timer code a lot, but never the GC metering code. I'm
> guessing Gregor is the main user of it.

I always run firefox with the GCTIMER option but I barely use GCMETER. It's also getting harder to maintain the GCMETER functionality with all the current changes. 

The current GCMETER information is very verbose and not too useful. I added once a fragmentation rate per compartment to get an idea what a copying collector would bring us. There is definitely a lot of potential in the data we collect.

I am OK with removing if Igor agrees or we could also revisit it's purpose
and make it more useful again.
(In reply to comment #4)
> I have used the shape dumping code once or twice, mostly just to get a sense
> of all the things that shapes are used for. This code is pretty easy to
> maintain, so I think it makes sense to keep it.

Agreed.  I'm a big fan of our Dump* functions + gdb; I definitely don't want to remove any of that code.
(In reply to comment #6)
> The current GCMETER information is very verbose and not too useful.

I agree with this. The accounting code was written for a very different GC initially. I have found that writing an ad-hock counters became more productive than trying to piggy-back on GCMETER. So lets remove it.
Great to hear.  I'll whip up a patch to nuke what we've discussed.
Attached patch rm metering codeSplinter Review
This patch removes 2557 lines of code (!!).
Assignee: general → luke
Attachment #540868 - Flags: review?(igor)
Comment on attachment 540868 [details] [diff] [review]
rm metering code

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

r+ with the comments below addressed.

::: js/src/jscompartment.h
@@ +289,5 @@
>  };
>  
> +namespace mjit { class JaegerCompartment; }
> +
> +}  /* namespace js */

Undo this change - lets focus here purely on metering removal.

::: js/src/jsgc.cpp
@@ +662,5 @@
> +        if (static_cast<GCMarker *>(trc)->conservativeDumpFileName)
> +            static_cast<GCMarker *>(trc)->conservativeRoots.append(thing);
> +        if (shift)
> +            static_cast<GCMarker *>(trc)->conservativeStats.unaligned++;
> +    }

Define a local variable like:

GCMarker *gcmarker = static_cast<GCMarker *>(trc);

and avoid repeating casts.

::: js/src/jsgc.h
@@ +293,1 @@
>      static size_t CountListLength(const ArenaHeader *aheader) {

IIRC CountListLength is used only for metering, so remove it if so.

@@ +1287,5 @@
>      js::gc::ConservativeGCStats conservativeStats;
>  #endif
>  
>  #ifdef JS_DUMP_CONSERVATIVE_GC_ROOTS
>      Vector<void *, 0, SystemAllocPolicy> conservativeRoots;

Merge 2 ifdefs into one

::: js/src/jshashtable.h
@@ -255,5 @@
>          tableCapacity = JS_BIT(sizeLog2);
>      }
>  
>  #ifdef DEBUG
> -    mutable struct Stats {

This stats are very useful at least for myself so do not remove them yet!

::: js/src/vm/String-inl.h
@@ -300,5 @@
>  
>  JS_ALWAYS_INLINE void
>  JSString::finalize(JSContext *cx)
>  {
> -    JS_ASSERT(!isStaticAtom() && !isShort());

I do not see why this assert is removed. Either keep it or explain in comments why it follows from other conditions.
Attachment #540868 - Flags: review?(igor) → review+
Blocks: 666042
(In reply to comment #11)
> I do not see why this assert is removed. Either keep it or explain in
> comments why it follows from other conditions.

Its post-dominated by a strictly strong assert.  I'll put it back though.
http://hg.mozilla.org/tracemonkey/rev/580ad572687b
Whiteboard: fixed-in-tracemonkey
This bug removed stuff that is not on in debug or release builds, along with stuff intentionally enabled by DEBUG ifdefs, and configurable at runtime startup via an envariable. The first category bitrots fast, and whether it's easy to recreate or not, should go.

The second category is different, not always easy to recreate. Who here knows what JSFunctionMeter measured? Reinventing that wheel is always better starting from a clean slate? Why?

/be
I believe code needs to justify its existence because its existence taxes JS hackers.  For metering code, this justification would seem to require (1) active use and (2) high re-materialization cost.

> The second category is different, not always easy to recreate. Who here
> knows what JSFunctionMeter measured?

If none of us know, then it seems to fail (1).  If (2) holds, then see below:

> Reinventing that wheel is always better starting from a clean slate? Why?

First, I am not advocating "always"; e.g., the hash table metering code was left because it sounds like Igor uses it.

However, because of the very real tax imposed on JS hackers, I am advocating "default to delete or, if there is a high re-materialization cost, stick it in a patch in a bug or in a separate repo".  The last option I think is quite pragmatic; we can write whatever extravagantly invasive metering code we like but only pay the tax (i.e., merge) when we want the goods.
(In reply to comment #15)
> I believe code needs to justify its existence because its existence taxes JS
> hackers. 

I second that. The tax can be quite expensive. A quick hack to add some temporary measuring code is a good way to learn the existing code IMO. On the other hand the presence of existing metering code destructs not only by various inevitable macros but also by extra mental efforts to understand what is metered and why. So I fully support the idea of letting the metering to rot in patches or extra repositories unless it is really used and has clear benefits with very low maintenance tax.
(In reply to comment #16)
> (In reply to comment #15)
> > I believe code needs to justify its existence because its existence taxes JS
> > hackers. 
> 
> I second that. The tax can be quite expensive. A quick hack to add some
> temporary measuring code is a good way to learn the existing code IMO.

My experience is also that the metering I want to do usually amounts to a quick hack. For metering code to live in the tree, it should probably reach the level of being a feature (in terms of quality and frequency of use), like the profiling Steve is working on. I think it's pretty easy to find out that we've removed too much--someone will complain that something they need is not there--and it won't cost too much to reverse the decision if that happens.
(In reply to comment #17)
> For metering code to live in the tree, it should probably reach
> the level of being a feature (in terms of quality and frequency of use)

... in which case the metering code will have to more than just compile, it will have tests and even documentation so that JS hackers will feel confident in both its meaning and the validity of its results without a full inspection.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Yet if Igor wants metering code in the hashtable implementation, it stays? Ok, you (Luke, I presume) have to bear the cost.

#ifdef NEVER code is bad, agreed. #ifdef DEBUG code, less so, if anyone looks. If no one looks, then my experience is that metered behavior diverges from original expectations, and tests don't often catch this (it's hard to measure O(n^2) or subtler effects).

It sounds like everyone would agree with metering that was an always-on feature used by something analogous to iostat(8). Right?

/be
(In reply to comment #17)
> My experience is also that the metering I want to do usually amounts to a
> quick hack.

I used to think this, until the umpteenth time I re-created some metering code I'd removed. If you re-live this karmic cycle, may you achieve nirvana sooner by my cautionary tale!

/be
(In reply to comment #20)
> It sounds like everyone would agree with metering that was an always-on
> feature used by something analogous to iostat(8). Right?

That sounds like what comment 17 and 18 were talking about so, 'yes' from me.
You need to log in before you can comment on or make changes to this bug.