Bug 110087 – WKR (new-commit-bot) is too slow

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

Apparently running "svn up" or "git svn fetch" on my 5 years old MBP is way too slow and takes multiple minutes to complete. I've found a solution that dramatically reduces this time that only works on a svn checkout.

[Ryosuke Niwa]

Created attachment 188816 [details]
Use svn log -r without updating checkout

[Ryosuke Niwa]

Created attachment 188817 [details]
Merged ToT

[Ryosuke Niwa]

I'm using the new implementation now. It's MUCH faster :D

[Ryosuke Niwa]

Created attachment 188834 [details]
Also use utf-8 encoding

[Adam Barth]

Comment on attachment 188834 [details]
Also use utf-8 encoding

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

> Tools/Scripts/webkitpy/tool/commands/newcommitbot.py:79
> +        for revision in range(self._last_svn_revision + 1, self._last_svn_revision + 10):

10?

> Tools/Scripts/webkitpy/tool/commands/newcommitbot.py:85
> +            if re.match(r'^\-+$', commit_log):
> +                continue

What is this looking for?

> Tools/Scripts/webkitpy/tool/commands/newcommitbot.py:89
> +            revision += 1

Doesn't the for-in-range handle incrementing the revision?

[Adam Barth]

Does it update it's working copy periodically?

[Ryosuke Niwa]

Comment on attachment 188834 [details]
Also use utf-8 encoding

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

>> Tools/Scripts/webkitpy/tool/commands/newcommitbot.py:79
>> +        for revision in range(self._last_svn_revision + 1, self._last_svn_revision + 10):
> 
> 10?

This is so that when there are many commits at once or something goes wrong, we don't end up spamming IRC with hundreds of messages.

>> Tools/Scripts/webkitpy/tool/commands/newcommitbot.py:85
>> +                continue
> 
> What is this looking for?

It's looking for empty entries that's filled with -------...
You get this when someone commits into branches.

>> Tools/Scripts/webkitpy/tool/commands/newcommitbot.py:89
>> +            revision += 1
> 
> Doesn't the for-in-range handle incrementing the revision?

Oops, yeah, we can get rid of this now.

[Ryosuke Niwa]

(In reply to comment #6)
> Does it update it's working copy periodically?

No. My plan is to have restart command do it.

[Adam Barth]

Comment on attachment 188834 [details]
Also use utf-8 encoding

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

>>> Tools/Scripts/webkitpy/tool/commands/newcommitbot.py:79
>>> +        for revision in range(self._last_svn_revision + 1, self._last_svn_revision + 10):
>> 
>> 10?
> 
> This is so that when there are many commits at once or something goes wrong, we don't end up spamming IRC with hundreds of messages.

OK.  Can you use a named constant and explain the "why" with a comment?

>>> Tools/Scripts/webkitpy/tool/commands/newcommitbot.py:85
>>> +                continue
>> 
>> What is this looking for?
> 
> It's looking for empty entries that's filled with -------...
> You get this when someone commits into branches.

Can you add a helper function with a descriptive name so that folks reading this code know what its supposed to do?

[Ryosuke Niwa]

(In reply to comment #9)
> (From update of attachment 188834 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188834&action=review
> 
> >>> Tools/Scripts/webkitpy/tool/commands/newcommitbot.py:79
> >>> +        for revision in range(self._last_svn_revision + 1, self._last_svn_revision + 10):
> >> 
> >> 10?
> > 
> > This is so that when there are many commits at once or something goes wrong, we don't end up spamming IRC with hundreds of messages.
> 
> OK.  Can you use a named constant and explain the "why" with a comment?

I'll add a named variable like _maximum_number_of_revisions_to_avoid_spamming_irc to avoid adding a comment.

> >>> Tools/Scripts/webkitpy/tool/commands/newcommitbot.py:85
> >>> +                continue
> >> 
> >> What is this looking for?
> > 
> > It's looking for empty entries that's filled with -------...
> > You get this when someone commits into branches.
> 
> Can you add a helper function with a descriptive name so that folks reading this code know what its supposed to do?

Sure. _is_empty_log?

[Adam Barth]

Sure.  Those names sound fine.

[Ryosuke Niwa]

Created attachment 197004 [details]
Fixed per comments

[Geoffrey Garen]

Comment on attachment 197004 [details]
Fixed per comments

r=me

[Ryosuke Niwa]

Committed r147984: <http://trac.webkit.org/changeset/147984>