469518 - Enable Reftest leak log in tinderbox (log)

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Serge Gautherie (:sgautherie)]

Currently, I use XPCOM_MEM_LEAK_LOG just fine on my local Windows.

But I don't know how to properly add it to the tinderbox config.

I guess Linux and MacOSX "could" simply use |env XPCOM_MEM_LEAK_LOG=1 ...|;
but what about Windows, which needs a separate |set XPCOM_MEM_LEAK_LOG=1| command ?

NB: This bug focuses on generating the log asap, automated parsing it could be dealt with later.

Comment 1

[Serge Gautherie (:sgautherie)]

Jeff, my first impression is that I'm not convinced that bug 468913 blocks this one: see bug 469581 comment 0 for example.

If there is to be a relation between the two, I would make it the other way round:
first make it work (= commandline) here, then automate it (= make) there.

Here, maybe I was thinking more about reusing/porting the mochitest runtests.py to handle reftests (too): see bug 456414 comment 3.

Comment 2

[Jeff Walden [:Waldo]]

I speculate we can reuse runtests.py and/or automation.py to do this, but first step is getting a makefile target so we can do whatever we want without involving IT every time, after changing tinderboxen to use the new command.  A makefile target is desirable in its own right, so there's not really a good reason not to make that the interface IT sees by just hacking the environment variable in beforehand (not to mention then you're not sharing existing leak detection infra).

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)
> Jeff, my first impression is that I'm not convinced that bug 468913 blocks this
> one

Eventually, bug 468913 created runreftest.py :-)

(In reply to comment #2)
> I speculate we can reuse runtests.py and/or automation.py to do this,

Where could we share leak parsing code between mochitests and reftests ? In automation.py ?

> after changing tinderboxen to use the new command.

Is that switch done yet ?

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Enable (bare) leak log

(In reply to comment #0)
> NB: This bug focuses on generating the log asap, automated parsing it could be
> dealt with later.

This patch adds the logging part (only).
I'd like to check it in as is, while we figure out the (automated) parsing part(s).

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> Where could we share leak parsing code between mochitests and reftests ? In
> automation.py ?

While there, I want to share runtests.py "# print test run times" part too.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Enable (bare) leak log [Checkin: See comment 10 & 15]

Av1, using the filename.

Looking for review from either of you...

Comment 7

[(not currently active) Ted Mielczarek]

I don't get the point of just spewing out the log. It doesn't seem to add any value? Why not just wait until you get the log parsing code hooked up, and we can figure out if we're leaking (and how much) and get some thresholds in place until we can drive the leaks to zero.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 363329 [details] [diff] [review]
(Av1a) Enable (bare) leak log
[Checkin: See comment 10 & 15]

r- per my last comment.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 363329 [details] [diff] [review]
(Av1a) Enable (bare) leak log
[Checkin: See comment 10 & 15]

Ok, after some IRC discussion, I'm ok with this. Serge says this will help him with filing leak bugs since he'll be able to see the output on tinderbox, despite it being purely informative.

+      if not os.path.exists(leakLogFile):
+        log.info("REFTEST WARNING | runreftest.py | Refcount logging is off, so leaks can't be detected!")

Since this is purely informative at the moment, I wouldn't bother with the warning.

Do we really need to check the leaks in the -silent case?

r=me with those addressed

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 363329 [details] [diff] [review]
(Av1a) Enable (bare) leak log
[Checkin: See comment 10 & 15]


http://hg.mozilla.org/mozilla-central/rev/ff4306c8fe81
Av1a, with comment 9 suggestion(s),
after further IRC discussion.

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> > I speculate we can reuse runtests.py and/or automation.py to do this,
> 
> Where could we share leak parsing code between mochitests and reftests ? In
> automation.py ?

Any answer to guide me ?

> > after changing tinderboxen to use the new command.
> 
> Is that switch done yet ?

I filed bug 480034.

While my previous checkin will already help people locally,
it's helpless on tinderboxes without that bug :-|

Comment 12

[Jeff Walden [:Waldo]]

automation.py is easy for everyone else to use because they're already using it, I say.

We'll get the tinderboxen changed, good news is we're making steady progress here.  The end is in sight!  :-)

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Move leak log parsing code to automation.py.in from runtests.py.in

The only code change is s/threshold = options.leakThreshold/threshold = leakThreshold/.
I'll update this code (step by step) in following patches.

Also, removes remaining trailing whitespaces.

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in [Checkin: See comment 17 & 29]

Bv1, unbitrotted after bug 480279.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 363329 [details] [diff] [review]
(Av1a) Enable (bare) leak log
[Checkin: See comment 10 & 15]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/587ef00fc422

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 366080 [details] [diff] [review]
(Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in
[Checkin: See comment 17 & 29]

Don't fix whitespace except in code you're already touching when you fix bugs.  Two-thirds of this patch is whitespace-only changes, and you shouldn't make reviewers read through such changes to get to the actual meat of a patch.


> ###############
> # RUN THE APP #
> ###############
> 
>+def processLeakLog(LEAK_REPORT_FILE, leakThreshold):

Usual naming convention has all-caps names as constants, so please rename to leakReportFile.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 366080 [details] [diff] [review]
(Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in
[Checkin: See comment 17 & 29]


http://hg.mozilla.org/mozilla-central/rev/ac355e4d7c61
(Bv1b) Move leak log parsing code to automation.py.in from runtests.py.in

Bv1a, without the whitespace fixes.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> >+def processLeakLog(LEAK_REPORT_FILE, leakThreshold):
> 
> Usual naming convention has all-caps names as constants, so please rename to
> leakReportFile.

This is planned for a following patch.

Comment 19

[Jeff Walden [:Waldo]]

(In reply to comment #18)
> This is planned for a following patch.

Sorry, I think that's unacceptable -- fix it in this patch, please.

Comment 20

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) "Fix" processLeakLog() arguments [Checkin: Comment 22 & 30]

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Early return in processLeakLog() [Checkin: See comment 26 & 31]

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 366375 [details] [diff] [review]
(Cv1) "Fix" processLeakLog() arguments
[Checkin: Comment 22 & 30]


http://hg.mozilla.org/mozilla-central/rev/546d72c0e307

Comment 23

[(not currently active) Ted Mielczarek]

bug 479225 went live, and I don't see any leaks on OS X:

INFO | (automation.py) | Application ran for: 0:01:03.794933

== BloatView: ALL (cumulative) LEAK STATISTICS
nsTraceRefcntImpl::DumpStatistics: 986 entries

REFTEST INFO | runreftest.py | Running tests: end.

From http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237229668.1237232605.9271.gz&fulltext=1

Is that just not reading the log properly, or is that what an empty log looks like?

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 366531 [details] [diff] [review]
(Dv1) Early return in processLeakLog()
[Checkin: See comment 26 & 31]

Ware conflicts with bug 482236, of course, when you land this and that.

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #23)
> is that what an empty log looks like?

It is!

Current status:
*FF 3.2: no (more) leaks :-)
*FF 3.1: bug 458844, expected as not fixed there.
*Bug 466376 is then specific to my computer :-<
*Bug 478520: I would still want to get it's leak log, even once only...

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 366531 [details] [diff] [review]
(Dv1) Early return in processLeakLog()
[Checkin: See comment 26 & 31]


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

after fixing context for
{
patching file build/automation.py.in
Hunk #1 FAILED at 406
1 out of 1 hunks FAILED
}

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This just happened on OS X 510.2 mozilla-central unit test:

== BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS

     |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
   0 TOTAL                                          -5        0 -1136421943        0 (188643.78 +/-     0.00) 1059492278        0 (21041.09 +/-     0.00)

nsTraceRefcntImpl::DumpStatistics: 1180 entries
TEST-UNEXPECTED-FAIL | runtests-leaks | negative leaks caught!

Related to this bug?

Comment 28

[Serge Gautherie (:sgautherie)]

(In reply to comment #27)
> Related to this bug?

No: that is bug 482236 -> bug 483500.

Comment 29

[(not currently active) Ted Mielczarek]

Comment on attachment 366080 [details] [diff] [review]
(Bv1a) Move leak log parsing code to automation.py.in from runtests.py.in
[Checkin: See comment 17 & 29]

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b6f75e02c3ad

Comment 30

[(not currently active) Ted Mielczarek]

Comment on attachment 366375 [details] [diff] [review]
(Cv1) "Fix" processLeakLog() arguments
[Checkin: Comment 22 & 30]

Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b3e8b948c73

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 366531 [details] [diff] [review]
(Dv1) Early return in processLeakLog()
[Checkin: See comment 26 & 31]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/88b434b46b84

Comment 32

[Serge Gautherie (:sgautherie)]

(In reply to comment #25)
> *FF 3.2: no (more) leaks :-)
> *FF 3.1: bug 458844, expected as not fixed there.

Ted, given current status of bug 458844 and bug 460548,
I'm proposing to do what we had for mochitests:
hack a threshold value in runreftest.py on 1.9.1 :-|
What do you think ?

Comment 33

[(not currently active) Ted Mielczarek]

As long as Waldo is ok with it, I'm fine with whatever you need to do.

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) Parse the log and support a leak threshold [Checkin: Comment 35]

And keep (improved) dumpLeakLog() around, as it might be (re)used in the future.

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 370745 [details] [diff] [review]
(Ev1) Parse the log and support a leak threshold
[Checkin: Comment 35]


http://hg.mozilla.org/mozilla-central/rev/c7979ff83ad7

Comment 36

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1) steps.py: support a leak threshold

Comment 37

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1-191) Parse the log, support and use a leak threshold [Checkin: Comment 39+41]

Same as Ev1, but with a default threshold.

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 371136 [details] [diff] [review]
(Gv1-191) Parse the log, support and use a leak threshold
[Checkin: Comment 39+41]

1.9.1 bug 458844 'nsMathML*' "leak":
FF/SM L: 725592 / 725592
FF/SM M: 725592 / 726704 (SM has bug 470710 too)
FF/SM W: 738120 / 725592

Comment 39

[Serge Gautherie (:sgautherie)]

Comment on attachment 371136 [details] [diff] [review]
(Gv1-191) Parse the log, support and use a leak threshold
[Checkin: Comment 39+41]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/76822b1b7bf2

Comment 40

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1-191) Add a specific crashtest leak threshold [Superseded by bug 391976]

1.9.1 bug 458844 'nsMathML*' crashtest "leak":
FF/SM L: 130844 / 130844
FF/SM M: 130612 / 131724 (SM has bug 470710 too)
FF/SM W: 130844 / 130612

Comment 41

[Serge Gautherie (:sgautherie)]

Comment on attachment 371136 [details] [diff] [review]
(Gv1-191) Parse the log, support and use a leak threshold
[Checkin: Comment 39+41]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/467ed372abcd
(Hv2-191) Remove forced leak threshold

now that bug 458844 is fixed.

Comment 42

[[e-mail] (:bhearsum)]

Attachment: (Fv1a) MozillaReftest leakThreshold support - unbitrotted

I tested this unbitrotted version of Fv1 today and everything went fine. I tried to get rid of the **kwargs digging but because UnittestBuildFactory explicitly passes env in kwargs we can't just name env and catch an override. Seems silly to block this bug on fixing that, so I just left this code as is and added leakThreshold to the addFactoryArguments call.

Comment 43

[Serge Gautherie (:sgautherie)]

Comment on attachment 373664 [details] [diff] [review]
(Fv1a) MozillaReftest leakThreshold support - unbitrotted


>diff -r d160c1da3488 unittest/steps.py

>@@ -281,15 +281,21 @@
>+    def __init__(self, test_name, leakThreshold=0, **kwargs):

I agree to change the default value for leakThreshold to 0 from None,
if you do it for MozillaMochitest too.

>+        # Because 'env' is explicitly passed into kwargs we cannot catch it
>+        # as a regular arg. Thus, we have to dig into kwargs
>+        if not "env" in kwargs:
>+            kwargs["env"] = {}

Why don't you use the same new code you changed in MozillaMochitest?
(As in bug 487496, I'm lost as why same feature gets different codes in different classes :-/)

>+        kwargs["EXTRA_TEST_ARGS"] = "--leak-threshold=%d" % leakThreshold

Shouldn't that be |kwargs["env"]["EXTRA_TEST_ARGS"]|?

I think it's wrong not to add(/keep) |if leakThreshold:| (to the whole block):
no need to pass this argument on unless it has a useful value.

(Nit: I would prefer you fix my bug 487496 comments there, before we proceed here.)

Comment 44

[[e-mail] (:bhearsum)]

Attachment: (Fv1a) reftest leakThreshold, the correct patch

Apologies for the noise, this is the correct patch, using leakThreshold=None like the Mochitest step.

Comment 45

[Serge Gautherie (:sgautherie)]

Comment on attachment 373691 [details] [diff] [review]
(Fv1a) reftest leakThreshold, the correct patch

>diff -r ed0581866a90 unittest/steps.py

>+    def __init__(self, test_name, leakThreshold=None, **kwargs):

If you s/None/0/ for MozillaMochitest in bug 487496,
then you could do it here/now too...

>+        # Because 'env' is explicitly passed into kwargs we cannot catch it
>+        # as a regular arg. Thus, we have to dig into kwargs

Please add this comment in bug 487496 too:
I do prefer to get exact same code and comment everywhere...

>+              "--leak-threshold=%d" % leakThreshold

I'd use an indentation of 4 spaces, but as you want.

Comment 46

[Serge Gautherie (:sgautherie)]

Comment on attachment 373691 [details] [diff] [review]
(Fv1a) reftest leakThreshold, the correct patch

r=me, +/- with comment 45 nits.

Comment 47

[[e-mail] (:bhearsum)]

(In reply to comment #46)
> (From update of attachment 373691 [details] [diff] [review])
> r=me, +/- with comment 45 nits.

What are you asking me to do? We're using None for the threshold in bug 487496, the second comment isn't an action item in this bug, and 2 spaces indentation is consistent with the rest of buildbotcustom for a continued line.

Comment 48

[Serge Gautherie (:sgautherie)]

(In reply to comment #47)

> What are you asking me to do? We're using None for the threshold in bug 487496,

Agreed, I made that comment before I eventually figured out that you changed your mind about switching to using 0.

> the second comment isn't an action item in this bug, and 2 spaces indentation

Well, you're adding a comment here that the same code (now checked in) in MozillaMochitest doesn't have, thus I'd (still) like you to add it at the other occurrence too.

> is consistent with the rest of buildbotcustom for a continued line.

Then, would you care to fix the/my 4 other occurrences in this file, which use 8 instead of 2? Thanks.

Comment 49

[[e-mail] (:bhearsum)]

(In reply to comment #48)
> > is consistent with the rest of buildbotcustom for a continued line.
> 
> Then, would you care to fix the/my 4 other occurrences in this file, which use
> 8 instead of 2? Thanks.

I am not currently on a mission to clean up and make consistent all of this code. I fixed what I was touching.

Comment 50

[Chris AtLee [:catlee]]

Comment on attachment 373691 [details] [diff] [review]
(Fv1a) reftest leakThreshold, the correct patch

>-    def __init__(self, test_name, **kwargs):
>-        ShellCommandReportTimeout.__init__(self, **kwargs)
>-        self.addFactoryArguments(test_name=test_name)
>-
>+    def __init__(self, test_name, leakThreshold=None, **kwargs):
>         self.name = test_name
>         self.command = ["make", test_name]
>         self.description = [test_name + " test"]
>         self.descriptionDone = [self.description[0] + " complete"]
>         self.super_class = ShellCommandReportTimeout
>+
>+        # Because 'env' is explicitly passed into kwargs we cannot catch it
>+        # as a regular arg. Thus, we have to dig into kwargs
>+        if leakThreshold:
>+            if not "env" in kwargs:
>+                kwargs["env"] = {}
>+            kwargs["env"]["EXTRA_TEST_ARGS"] = \
>+              "--leak-threshold=%d" % leakThreshold
>+        ShellCommandReportTimeout.__init__(self, **kwargs)
>+        self.addFactoryArguments(test_name=test_name,
>+                                 leakThreshold=leakThreshold)

I think we can clean this up a bit by having env=None in __init__, and then calling ShellCommandReportTimeout.__init__(self, env=env, **kwargs). If you have a parameter called 'env' in your function signature, it will grab anything called 'env' from keyword parameters passed in.

Comment 51

[[e-mail] (:bhearsum)]

Attachment: (Fv1b) reftest leakThreshold, without kwargs manipulation [Checkin: Comment 52]

Comment 52

[[e-mail] (:bhearsum)]

Comment on attachment 374126 [details] [diff] [review]
(Fv1b) reftest leakThreshold, without kwargs manipulation
[Checkin: Comment 52]

changeset:   265:e1ad076706f0

Comment 53

[[e-mail] (:bhearsum)]

Comment on attachment 374126 [details] [diff] [review]
(Fv1b) reftest leakThreshold, without kwargs manipulation
[Checkin: Comment 52]

production-master has been reconfig'ed for this patch.