Bug 92913 – Add Perf EWS IRC bot

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Per Adam's suggestion, I'm going to add an experimental perf EWS IRC bot before integrating it into Bugzilla.

[Ryosuke Niwa]

Created attachment 155934 [details]
work in progress

[Ryosuke Niwa]

Created attachment 155935 [details]
Added the forgotten perfalizer.py

[Ryosuke Niwa]

Created attachment 156847 [details]
work in progress 3

[Ryosuke Niwa]

Created attachment 157075 [details]
work in progress 4

[Ryosuke Niwa]

Created attachment 157081 [details]
work in progress 5

[Ryosuke Niwa]

Created attachment 157087 [details]
Adds the initial implementation of perfalizer

[Ryosuke Niwa]

Unfortunately, there isn't much we can test here because it uses a lot of methods that themselves cannot be tested.

[Dirk Pranke]

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?

[Ryosuke Niwa]

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.

[Dirk Pranke]

(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 ...

[Ryosuke Niwa]

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).

[Dirk Pranke]

(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)?

[Ryosuke Niwa]

Created attachment 157334 [details]
Added some tests per Dirk's comment

[Dirk Pranke]

Comment on attachment 157334 [details]
Added some tests per Dirk's comment

thanks! coverage makes me happier ...

[Ryosuke Niwa]

Committed r125123: <http://trac.webkit.org/changeset/125123>