Serge Gautherie (:sgautherie) Reporter
	Description • 16 years ago

Bloat tinderboxes need to use XPCOM_MEM_BLOAT_LOG to do a full diff between runs.

But regular test boxes (and developers/testers) could use XPCOM_MEM_LEAK_LOG only.

	Serge Gautherie (:sgautherie) Reporter
	Comment 1 • 16 years ago

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081211 Minefield/3.2a1pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/0d2bceefc012)

This will "unbloat" (speed and data) the regular test run use case.

*****

Before checking this in, '--bloat-log' needs to be added to the bloat tinderboxes config, but I don't know where that is.
Helpwanted.

	Serge Gautherie (:sgautherie) Reporter
	Comment 2 • 16 years ago

"Same" as Av1, to ease review.

	Serge Gautherie (:sgautherie) Reporter
	Comment 3 • 16 years ago

Av1-w, with nits fixed.

NB: I'll update the full patch before checkin.

	(not currently active) Ted Mielczarek
	Comment 4 • 16 years ago

Comment on attachment 352873 [details] [diff] [review]
(Av1) <runtests.py.in>

(In reply to comment #1)
> 
> Before checking this in, '--bloat-log' needs to be added to the bloat
> tinderboxes config, but I don't know where that is.
> Helpwanted.

What do you mean here? The "leak test" boxes that do debug builds don't use runtests.py, they use this file (which uses automation.py): http://mxr.mozilla.org/mozilla-central/source/build/leaktest.py.in

Passing this review off to Waldo, as he wrote all the leak-checking code.

	Serge Gautherie (:sgautherie) Reporter
	Comment 5 • 16 years ago

(In reply to comment #4)
> What do you mean here? The "leak test" boxes that do debug builds don't use

Er, checking the current waterfalls for bloat/leak/test boxes,
I feel a little confused (by my previous comment), then I do agree with you :->

> runtests.py, they use this file (which uses automation.py):
> http://mxr.mozilla.org/mozilla-central/source/build/leaktest.py.in

Then this patch does not depend on anything else :-)

	Jeff Walden [:Waldo] Assignee
	Comment 6 • 16 years ago

Comment on attachment 352873 [details] [diff] [review]
(Av1) <runtests.py.in>

This change seems vastly larger than necessary.  I don't recall a particular reason XPCOM_MEM_BLOAT_LOG was used rather than XPCOM_MEM_LEAK_LOG; the outputs are essentially the same, except that bloat outputs lines for stuff that doesn't leak and includes an extra header or two, going by code inspection and memory.  Why not just change the leak-detection code to handle leak logs and change the environment variable entirely?

	Serge Gautherie (:sgautherie) Reporter
	Comment 7 • 16 years ago

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081217 SeaMonkey/2.0a3pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/4a4a42520901
 +http://hg.mozilla.org/comm-central/rev/877154dd96a2 + bug 469606 patch)

Av1(a), with comment 6 suggestion(s).

	Serge Gautherie (:sgautherie) Reporter
	Comment 8 • 16 years ago

"Same" as Av2, to ease review.

	Jeff Walden [:Waldo] Assignee
	Comment 9 • 16 years ago

Comment on attachment 353643 [details] [diff] [review]
(Av2) <runtests.py.in>

I don't get the point of such large changes.  Switch the variable name, maybe, but none of the leak-detection code should need to be changed, as far as I can tell from the leak-printing code.  The outputs in the two modes of operation are similar enough that it shouldn't be necessary to make any other changes, as far as I can tell.

	Serge Gautherie (:sgautherie) Reporter
	Comment 10 • 16 years ago

(In reply to comment #9)
> The outputs in the two modes of operation are similar enough [...]

They are not (since I changed it):
the leak one has no 'TOTAL' line (anymore) when there are no leaks.
Thus the added validating step is also useful to compensate this (lack/optim.).
Then there are just a few additional nits.

As much as I agree the config option was ... optional,
I actually fail to see what you call "large" change :-|

	Jeff Walden [:Waldo] Assignee
	Comment 11 • 16 years ago

So how about if we add the TOTAL line to LEAK_LOG when there are no leaks?  That seems like a needless difference between the two modes.

	Serge Gautherie (:sgautherie) Reporter
	Comment 12 • 16 years ago

(In reply to comment #11)

On one hand, I removed it and I don't think it should be readded.
On the other hand, if you do want something to be added, I would easily agree for some kind of "INFO" line saying "no leaks"...

	Jeff Walden [:Waldo] Assignee
	Comment 13 • 16 years ago

On another note, what we have works.  The only thing that's possibly broken is that you have to scroll back a few extra screens to get to execution output.  If it requires that much effort to switch just to avoid printing lines for non-leaky things, I don't particularly think it's worthwhile.

Speaking of which, why not just filter out the non-leak lines, save for the TOTAL line perhaps, when logging the entire file?  That's easy and non-invasive and doesn't waste too much of anyone's time.

	Serge Gautherie (:sgautherie) Reporter
	Comment 14 • 16 years ago

(In reply to comment #13)
> On another note, what we have works.  The only thing that's possibly broken is
> that you have to scroll back a few extra screens to get to execution output. 

It sure works, but the current output is mainly bloat:
wastes time to process, occupies useless screen/disk/network space, makes looking at what is before and after a pain, etc.

> If it requires that much effort to switch just to avoid printing lines for
> non-leaky things, I don't particularly think it's worthwhile.

I'm still baffled that you see this patch as "so big" whereas I see it rather "straightforward" (I'm not saying "trivial", mind.) :-(
Moreover, I would think that _I_ eventually/already made the effort to dig into the issue and create the patch...

> Speaking of which, why not just filter out the non-leak lines, save for the
> TOTAL line perhaps, when logging the entire file?  That's easy and non-invasive
> and doesn't waste too much of anyone's time.

Re-implementing the Leak-Only feature ? Duplicating code/feature would not be my first choice...
It looks to me it would take much less of your and my time to accept (one of) the current patch (unless you think it doesn't work fine) than having this "debate" :-/

NB: I guess I just really didn't expect such comments about a self-contained patch/file :-|

	Jeff Walden [:Waldo] Assignee
	Comment 15 • 16 years ago

Processing time and hard disk/screen/network space waste are pretty negligible overall.

Sorry, rejiggering the log-parsing code isn't something I think is that worthwhile.  More relevant to me is that I don't think it's a great use of *my* time to *review* such a change.

Adding a test to filter out no-leak lines in the log-dumping loop doesn't affect the parsing at all (so very little difficulty) and is only about a two-line change; I ask again, why isn't this a better way to implement this?

	Shawn Wilsher :sdwilsh
	Comment 16 • 16 years ago

(In reply to comment #15)
> Processing time and hard disk/screen/network space waste are pretty negligible
> overall.
Really?  I absolutely hate having to parse a bazillion lines of non-leaking output when I'm trying to see the details on what leaked on tinderbox.  The stuff that isn't leaking isn't really important it turns out...

> Sorry, rejiggering the log-parsing code isn't something I think is that
> worthwhile.  More relevant to me is that I don't think it's a great use of *my*
> time to *review* such a change.
Then punt to another reviewer?  Clearly, there exists a set of people who find this useful...

	Jeff Walden [:Waldo] Assignee
	Comment 17 • 16 years ago

(In reply to comment #16)
> Really?  I absolutely hate having to parse a bazillion lines of non-leaking
> output when I'm trying to see the details on what leaked on tinderbox.  The
> stuff that isn't leaking isn't really important it turns out...

Why are you parsing it at all rather than just searching for TEST-UNEX or similar?

> Then punt to another reviewer?

Because even considering that, I don't think it's good to rejigger working logging code with no known deficiencies when it's not necessary.  It's not necessary because we can just filter out those lines at output time, rather than changing log-parsing code.

I repeat yet again:

Why are we modifying log-parsing code *that works* when we could just modify *log output* code to get rid of the non-leak lines?

	Jeff Walden [:Waldo] Assignee
	Comment 18 • 16 years ago

This is what I wanted.

Is it simpler?  Yes.

Does it change the existing log-parsing code?  No.  Does that make the change easier to understand?  Yes.

Does it change the existing log-output code?  Yes.  Is it limited only to changing that code, and no other code?  Yes.

Any complaints about this?  I don't see how this could possibly make anyone unhappy, but maybe I'm blind.

	Jeff Walden [:Waldo] Assignee
	Comment 19 • 16 years ago

I wrote this in exactly the time between my last comment and the time the patch was posted, 16:34 total.  I very much doubt the previous patches took that little time to write and test *precisely because* they modified the log-parsing code.

	Serge Gautherie (:sgautherie) Reporter
	Comment 20 • 16 years ago

(In reply to comment #17)
> Why are you parsing it at all rather than just searching for TEST-UNEX or
> similar?

Because when a leak decreases, there are no TEST-UNEX lines !
Because when I run any test locally, that output fills up my console !
etc.

(In reply to comment #18)
> Is it simpler?  Yes.

Does it have all/any of the "nits" fix ? No.

> Does it change the existing log-parsing code?  No.  Does that make the change
> easier to understand?  Yes.

Does it remove the now dead code in the existing log-parsing code ? No.
Is it easier to understand duplicated or useless code ? No.

> Does it change the existing log-output code?  Yes.  Is it limited only to
> changing that code, and no other code?  Yes.

Does it (re)use the Core Leak-Only mode ? No.
Does it add the validation of the leak log ? No.

> Any complaints about this?  I don't see how this could possibly make anyone
> unhappy, but maybe I'm blind.

Will this remove most (but not all !) of the useless output ? Yes.

(In reply to comment #19)
> I wrote this in exactly the time between my last comment and the time the patch
> was posted, 16:34 total.  I very much doubt the previous patches took that

Sure, doing 1-2 Copy & Paste is so much faster !
Not to say that there is not even a single line of comment...

> little time to write and test *precisely because* they modified the log-parsing
> code.

Do I think this is Quick and "Dirty" ? Yes.

Now, I'm the contributer and you're the reviewer: you win :-/

	Jeff Walden [:Waldo] Assignee
	Comment 21 • 16 years ago

Ideological purity is not a goal.  Nor is using a different leak-logging mode just because it exists.  What we have works, save for the extra output, and I am loathe to change it when a small tweak will work just as well as the larger one previously proposed.

	Serge Gautherie (:sgautherie) Reporter
	Comment 22 • 16 years ago

(In reply to comment #21)
> Ideological purity is not a goal.  Nor is using a different leak-logging mode
> just because it exists.

I don't know about that.
But I know I can't imagine what the interest is to close a file then reopen it 2 lines later, for example.
Nor, in your code, what is even the point in reading the file twice.

> What we have works, save for the extra output, and I am loathe to change it

Working is very fine ! Yet it doesn't mean it can't be improved/enhanced...

> when a small tweak will work just as well as the larger one previously proposed.

That, I'll never understand, when the *only* change needed is to move the 2 "no leaks detected!" and "missing output" lines around ... and this added validation:
1) is easier then your proposal and 2) gives an enhanced detection.

What makes my patch look larger than that is only some "nit" fixes while there and, for the full patch, the change in indentation.

	(not currently active) Ted Mielczarek
	Comment 23 • 16 years ago

Comment on attachment 354130 [details] [diff] [review]
I hate hg

+      if matches and \
+         matches.group("name") != "TOTAL" and \
+         int(matches.group("numLeaked")) == 0:

This condition is a little confusing to read, I wonder if you could group it or something to make it clearer?

	Jeff Walden [:Waldo] Assignee
	Comment 24 • 16 years ago

I grouped the condition and moved the numLeaked test before the != TOTAL test; I think this makes it clearer it's filtering out lines that have zero leaks but aren't the TOTAL line.

http://hg.mozilla.org/mozilla-central/rev/1a4e256b6894

I'll land in 191 sometime later today or maybe tomorrow; since it only affects output and not leak processing there's basically no danger of error.

	Jeff Walden [:Waldo] Assignee
	Comment 25 • 16 years ago

191 as well now:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/74fbbcf32bdf

	Serge Gautherie (:sgautherie) Reporter
	Comment 26 • 16 years ago

Reopening, per bug 483917 comment 2:
I still believe comment 0 is the correct way to go.

	Serge Gautherie (:sgautherie) Reporter
	Comment 27 • 16 years ago

Jeff, ping about comment 26?

	Jeff Walden [:Waldo] Assignee
	Comment 28 • 15 years ago

Disagree.  What we have works.  More useful things to spend time on than a change that won't have a notable effect on what developers see.