Bug 92913

Summary: Add Perf EWS IRC bot
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dbates, dpranke, eric, haraken, morrita, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 92912, 93042, 93296    
Bug Blocks: 77037    
Attachments:
Description Flags
work in progress
none
Added the forgotten perfalizer.py
none
work in progress 3
none
work in progress 4
none
work in progress 5
none
Adds the initial implementation of perfalizer
none
Added some tests per Dirk's comment dpranke: review+

Description Ryosuke Niwa 2012-08-01 15:12:35 PDT
Per Adam's suggestion, I'm going to add an experimental perf EWS IRC bot before integrating it into Bugzilla.
Comment 1 Ryosuke Niwa 2012-08-01 17:43:34 PDT
Created attachment 155934 [details]
work in progress
Comment 2 Ryosuke Niwa 2012-08-01 17:44:38 PDT
Created attachment 155935 [details]
Added the forgotten perfalizer.py
Comment 3 Ryosuke Niwa 2012-08-06 20:21:13 PDT
Created attachment 156847 [details]
work in progress 3
Comment 4 Ryosuke Niwa 2012-08-07 18:26:08 PDT
Created attachment 157075 [details]
work in progress 4
Comment 5 Ryosuke Niwa 2012-08-07 18:47:58 PDT
Created attachment 157081 [details]
work in progress 5
Comment 6 Ryosuke Niwa 2012-08-07 19:25:20 PDT
Created attachment 157087 [details]
Adds the initial implementation of perfalizer
Comment 7 Ryosuke Niwa 2012-08-07 19:25:55 PDT
Unfortunately, there isn't much we can test here because it uses a lot of methods that themselves cannot be tested.
Comment 8 Dirk Pranke 2012-08-07 21:56:13 PDT
Comment on attachment 157087 [details]
Adds the initial implementation of perfalizer

View in context: https://bugs.webkit.org/attachment.cgi?id=157087&action=review

Disclaimer: I don't really know the IRCbot code at all, so some of these questions might be a bit off-base.

Also, I'd want to give a bit more thought to if/how we can test this. I understand the comment about this being untestable, but python's static checking is pretty weak. We might want to consider at least putting in something that runs lint-webkitpy over this to catch more errors.

> Tools/Scripts/webkitpy/common/system/filesystem.py:80
> +

You should probably add a routine to filesystem_mock as well ...

> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:49
> +    def _copy_build_product_without_patch(self, postfix):

'postfix' isn't a great name here; at the least it should probably be "suffix", but even better would be something like "directory_suffix" or "build_dir_suffix"

> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:52
> +        self._build_path = filesystem.dirname(self._port._build_path())

It's a bit confusing that self._build_path != self._port._build_path() ; maybe pick a different name for the former? This is especially true since you end up adding the configuration dirs back on down below.

> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:58
> +                filesystem.join(self._build_path_without_patch, configuration))

I wonder if something like rsync -a --delete would be much of a win here over rm + cp. I suppose it depends on how often you'll be building ...

> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:64
> +        if not self._patch.committer() and not self._patch.attacher().can_commit:

Should we have a comment here about why we need a committer here?

> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:135
> +        self.run_webkit_patch(command)

are this and the functions below used anywhere and/or needed? They seem unrelated and possibly just copy&paste from a different command?
Comment 9 Ryosuke Niwa 2012-08-07 22:04:52 PDT
Comment on attachment 157087 [details]
Adds the initial implementation of perfalizer

View in context: https://bugs.webkit.org/attachment.cgi?id=157087&action=review

>> Tools/Scripts/webkitpy/common/system/filesystem.py:80
>> +
> 
> You should probably add a routine to filesystem_mock as well ...

Possibly but filesystem_mock doesn't seem to have methods not used in tests so we can probably wait 'til we need one in tests.

>> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:52
>> +        self._build_path = filesystem.dirname(self._port._build_path())
> 
> It's a bit confusing that self._build_path != self._port._build_path() ; maybe pick a different name for the former? This is especially true since you end up adding the configuration dirs back on down below.

Oh oops, yeah, I should have named it self._build_directory.

>> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:58
>> +                filesystem.join(self._build_path_without_patch, configuration))
> 
> I wonder if something like rsync -a --delete would be much of a win here over rm + cp. I suppose it depends on how often you'll be building ...

Maybe but copytree is so much slower that the time spent in rmtree doesn't really matter.

>> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:64
>> +        if not self._patch.committer() and not self._patch.attacher().can_commit:
> 
> Should we have a comment here about why we need a committer here?

What kind of comment are we going to add?

>> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:135
>> +        self.run_webkit_patch(command)
> 
> are this and the functions below used anywhere and/or needed? They seem unrelated and possibly just copy&paste from a different command?

Yes. Otherwise PatchAnalysisTask is going to blow up.
Comment 10 Dirk Pranke 2012-08-07 22:39:05 PDT
(In reply to comment #9)
> (From update of attachment 157087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157087&action=review
> 
> >> Tools/Scripts/webkitpy/common/system/filesystem.py:80
> >> +
> > 
> > You should probably add a routine to filesystem_mock as well ...
> 
> Possibly but filesystem_mock doesn't seem to have methods not used in tests so we can probably wait 'til we need one in tests.
> 

Sure.

> >> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:64
> >> +        if not self._patch.committer() and not self._patch.attacher().can_commit:
> > 
> > Should we have a comment here about why we need a committer here?
> 
> What kind of comment are we going to add?
>

Why do you need to be a committer (or does the patch need to be uploaded by a committer)? It's not obvious ...
Comment 11 Ryosuke Niwa 2012-08-07 22:46:57 PDT
Comment on attachment 157087 [details]
Adds the initial implementation of perfalizer

View in context: https://bugs.webkit.org/attachment.cgi?id=157087&action=review

>>>> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:64
>>>> +        if not self._patch.committer() and not self._patch.attacher().can_commit:
>>> 
>>> Should we have a comment here about why we need a committer here?
>> 
>> What kind of comment are we going to add?
> 
> Why do you need to be a committer (or does the patch need to be uploaded by a committer)? It's not obvious ...

Oh, that's merely because I don't trust those patches to be ran on my Mac mini at the moment.
I'm not sure if adding a comment like "rniwa doesn't trust patches submitted by non-committers" is any more helpful than the code itself given that nobody else is going to run this thing
(surely, we can't have two IRC bots named perfalizer).
Comment 12 Dirk Pranke 2012-08-08 14:45:29 PDT
(In reply to comment #11)
> (From update of attachment 157087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157087&action=review
> 
> >>>> Tools/Scripts/webkitpy/tool/commands/perfalizer.py:64
> >>>> +        if not self._patch.committer() and not self._patch.attacher().can_commit:
> >>> 
> >>> Should we have a comment here about why we need a committer here?
> >> 
> >> What kind of comment are we going to add?
> > 
> > Why do you need to be a committer (or does the patch need to be uploaded by a committer)? It's not obvious ...
> 
> Oh, that's merely because I don't trust those patches to be ran on my Mac mini at the moment.
> I'm not sure if adding a comment like "rniwa doesn't trust patches submitted by non-committers" is any more helpful than the code itself given that nobody else is going to run this thing
> (surely, we can't have two IRC bots named perfalizer).

I see, I thought this was some sort of security thing (I think we have this check on the other EWS bots, for example).

At any rate, as far as testing goes, can you put in some tests that at least stub out the executive enough to make sure the code executes *at all* (i.e., getting covered, even if we're not testing correctness)?
Comment 13 Ryosuke Niwa 2012-08-08 16:53:32 PDT
Created attachment 157334 [details]
Added some tests per Dirk's comment
Comment 14 Dirk Pranke 2012-08-08 16:55:22 PDT
Comment on attachment 157334 [details]
Added some tests per Dirk's comment

thanks! coverage makes me happier ...
Comment 15 Ryosuke Niwa 2012-08-08 17:19:29 PDT
Committed r125123: <http://trac.webkit.org/changeset/125123>