Closed Bug 469509 Opened 11 years ago Closed 10 years ago

runtests.py.in : don't print leak lines for classes that aren't leaked

Categories

(Testing :: Mochitest, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1b3

People

(Reporter: sgautherie, Assigned: Waldo)

References

Details

(Keywords: fixed1.9.1, polish)

Attachments

(3 files, 4 obsolete files)

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.
Attached patch (Av1) <runtests.py.in> (obsolete) — Splinter Review
[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.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Attachment #352873 - Flags: review?(ted.mielczarek)
Attached patch (Av1-w) <runtests.py.in> (obsolete) — Splinter Review
"Same" as Av1, to ease review.
Attached patch (Av1a-w) <runtests.py.in> (obsolete) — Splinter Review
Av1-w, with nits fixed.

NB: I'll update the full patch before checkin.
Attachment #352874 - Attachment is obsolete: true
Attachment #352873 - Flags: review?(ted.mielczarek) → review?(jwalden+bmo)
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.
(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 :-)
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?
Attachment #352873 - Flags: review?(jwalden+bmo) → review-
[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).
Attachment #352873 - Attachment is obsolete: true
Attachment #353643 - Flags: review?(jwalden+bmo)
"Same" as Av2, to ease review.
Attachment #353643 - Attachment is obsolete: true
Attachment #353643 - Flags: review?(jwalden+bmo)
Attachment #352913 - Attachment is obsolete: true
Attachment #353643 - Attachment is obsolete: false
Attachment #353643 - Flags: review?(jwalden+bmo)
Attachment #353643 - Flags: review?(jwalden+bmo) → review-
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.
(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 :-|
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.
(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"...
Depends on: 448934
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.
(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 :-|
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?
(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...
(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?
Attached patch What I want (obsolete) — Splinter Review
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.
Attachment #354129 - Flags: review?(ted.mielczarek)
Attached patch I hate hgSplinter Review
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.
Attachment #354129 - Attachment is obsolete: true
Attachment #354130 - Flags: review?(ted.mielczarek)
Attachment #354129 - Flags: review?(ted.mielczarek)
(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 :-/
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.
(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.
Attachment #354130 - Flags: review?(ted.mielczarek) → review+
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?
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.
Assignee: sgautherie.bz → jwalden+bmo
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Summary: runtests.py.in : use XPCOM_MEM_LEAK_LOG by default → runtests.py.in : don't print leak lines for classes that aren't leaked
Depends on: 483917
Reopening, per bug 483917 comment 2:
I still believe comment 0 is the correct way to go.
Blocks: 483917
Status: RESOLVED → REOPENED
No longer depends on: 483917
Resolution: FIXED → ---
Jeff, ping about comment 26?
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.