1190466 - Rewrite tools/rb/find-leakers.pl in Python

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

This script is quite simple so we should just rewrite it in Python. My main motivation here is that the error checking could be improved. It fails to notice if an object is destroyed that was never created, for instance.

Comment 1

[Nicholas Nethercote [inactive]]

Does this script get any kind of regular (i.e. automated) testing?

Comment 2

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> Does this script get any kind of regular (i.e. automated) testing?

Nope. I use it myself every few months. I'd be surprised if anybody else still uses it, but maybe they do.

This analyzes a refcount log and prints out objects that aren't destroyed by the end of the log.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I used it this week.

Comment 4

[Nicholas Nethercote [inactive]]

I asked about testing because I'm just wondering how we would test the new script to make sure it's at least as functional and correct as the old script.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> I asked about testing because I'm just wondering how we would test the new
> script to make sure it's at least as functional and correct as the old
> script.

I was just going to try to decipher the script to see exactly what it supports, then test those against a log. It is a very simple script.

Comment 6

[Aidin Gharibnavaz]

Attachment: 1190466.patch

The attachment is the Python version of the "find-leakers" script. It has been written in Python2.7.

It's the exact algorithm of the Perl version. I just did a little clean up in the code structure.

Please check it, and let me know if there was anything wrong, or there's any new requirement I should implement (:

Comment 7

[Andrew McCreight [:mccr8]]

Great! I'll look through this on Monday.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8648330 [details] [diff] [review]
1190466.patch

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

Thanks for writing this! I have a number of review comments, but they are mostly just little things. The patch looks good over all. Just upload a new version of this patch with the review comments addressed when you get a chance and then it can be checked in.

Please call this file find_leakers.py instead of find-leakers.py to match the other Python files in this directory.

Also, please delete find-leakers.pl as it shouldn't be needed any more.

::: tools/rb/find-leakers.py
@@ +1,2 @@
> +#!/usr/bin/python
> +

Please remove this blank line.

@@ +4,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# This script process a `refcount' log, and finds out if any object leaked.

Thanks for writing this comment. I have a few minor grammatical corrections. "process" should be "processes".

@@ +6,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# This script process a `refcount' log, and finds out if any object leaked.
> +# It simply goes through the log, finds `AddRef' or `Ctor' lines, and then
> +# see if they `Release' or `Dtor'. If not, it report them as leaks.

"see" should be "sees" and "report" should be "reports".

@@ +16,5 @@
> +def print_output(allocation, obj_to_class):
> +    '''Formats and prints output.'''
> +    items = []
> +    for obj, count, in allocation.iteritems():
> +        # Adding items to a list, so we can sort them.

I think this comment and the next comment ("Sorting by count") aren't really needed, so please remove them.

@@ +37,5 @@
> +    obj_to_class = {}
> +
> +    for log_line in log_lines:
> +        if not log_line.startswith('<'):
> +            # Not a line we're interested in.

This comment isn't really needed.

@@ +40,5 @@
> +        if not log_line.startswith('<'):
> +            # Not a line we're interested in.
> +            continue
> +
> +        (class_name,

Maybe just make all of this one line.

@@ +46,5 @@
> +         ignore,
> +         operation,
> +         count,) = log_line.strip('\r\n').split(' ')
> +
> +        # for AddRef/Release `count' is the refcount,

This comment can just be one line.

@@ +54,5 @@
> +           operation == 'Ctor'):
> +            # Examples:
> +            #     <nsStringBuffer> 0x01AFD3B8 1 AddRef 1
> +            #     <PStreamNotifyParent> 0x08880BD0 8 Ctor (20)
> +            if class_name not in class_count:

I think you can replace this block with
class_count[class_name] = class_count.setdefault(class_name, 0) + 1

@@ +61,5 @@
> +                class_count[class_name] += 1
> +            allocation[obj] = class_count[class_name]
> +            obj_to_class[obj] = class_name
> +
> +        if ((operation == 'Release' and count == '0') or

This should be an elsif, because if it matches the first if (with the 'AddRef' etc) it won't match the second.

@@ +67,5 @@
> +            # Examples:
> +            #     <nsStringBuffer> 0x01AFD3B8 1 Release 0
> +            #     <PStreamNotifyParent> 0x08880BD0 8 Dtor (20)
> +            if obj not in allocation:
> +                print "An object is released that wasn't allocated!", obj, "@", class_name

I think "is released" should be "was released".

@@ +84,5 @@
> +    print "If `log-file' provided, it will read that as the input log."
> +    print "Else, it will read the stdin as the input log."
> +    print
> +
> +def main():

Should main() maybe get inlined into the block below? Either way is fine, though.

@@ +100,5 @@
> +        print_usage()
> +
> +if __name__ == '__main__':
> +    main()
> +

Please remove the blank line at the end of the file.

Comment 9

[Andrew McCreight [:mccr8]]

I ran the script on an 800mb refcount log file, and it was slightly slower than the Perl version, but it just takes like 3 seconds instead of 2, so that's totally fine.

Comment 10

[Aidin Gharibnavaz]

Attachment: 1190466.patch

Thanks for the review. You have a keen eye of details (:
I fixed those things. And, sorry for the grammatical errors :$

I just not agree with these three comments. Please check them again, and see if they're OK to you.

> I think this comment and the next comment ("Sorting by count") aren't really needed, so please remove them.

I think at the first sight, it's not very clear what this block doing. So it worth it to have two extra lines of comment.

> Maybe just make all of this one line.
> This comment can just be one line.

In Python standard code style (PEP 0008) maximum line length is 79 characters. I followed that, as it said on Mozilla Code Style.

> Should main() maybe get inlined into the block below? Either way is fine, though.

That's a common practice. So if someone importing this module, it won't get executed. I prefer to keep that, if you don't mind.

Yes, I noticed that it's slower than the perl version. But couldn't find a way to improve it.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Aidin Gharibnavaz from comment #10)
> I fixed those things. And, sorry for the grammatical errors :$
No problem, that is what reviews are for!

> I just not agree with these three comments. Please check them again, and see
> if they're OK to you.
Thanks!

> I think at the first sight, it's not very clear what this block doing. So it
> worth it to have two extra lines of comment.
Sounds good.
 
> In Python standard code style (PEP 0008) maximum line length is 79
> characters. I followed that, as it said on Mozilla Code Style.
Okay, great. I don't really know anything about the standard Python way to do things. :)

> > Should main() maybe get inlined into the block below? Either way is fine, though.
> That's a common practice. So if someone importing this module, it won't get
> executed. I prefer to keep that, if you don't mind.
That's fine. I mean, inline the code in main() into the "if __name__ == '__main__':" block, so it shouldn't affect what is run, but maybe that's not the usual Python style.
 
> Yes, I noticed that it's slower than the perl version. But couldn't find a
> way to improve it.

Yeah, just a loop that only checked for a leaking '>' was already quite a bit slower. Oh well.

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 8649362 [details] [diff] [review]
1190466.patch

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

Sorry, I meant to review this yesterday but I forgot about it.

Comment 13

[Andrew McCreight [:mccr8]]

This script is not run anywhere on TreeHerder, so it doesn't need a try run.

Comment 14

[Andrew McCreight [:mccr8]]

Thanks again for your patch.

Comment 15

[Aidin Gharibnavaz]

Thank you, for the review (:

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/896db7d401d3

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/896db7d401d3

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/896db7d401d3