Bug 110087 - WKR (new-commit-bot) is too slow
Summary: WKR (new-commit-bot) is too slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-18 01:15 PST by Ryosuke Niwa
Modified: 2013-04-08 23:43 PDT (History)
7 users (show)

See Also:


Attachments
Use svn log -r without updating checkout (3.67 KB, patch)
2013-02-18 01:17 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Merged ToT (3.60 KB, patch)
2013-02-18 01:21 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Also use utf-8 encoding (3.66 KB, patch)
2013-02-18 02:32 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per comments (4.12 KB, patch)
2013-04-08 23:39 PDT, Ryosuke Niwa
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-02-18 01:15:01 PST
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.
Comment 1 Ryosuke Niwa 2013-02-18 01:17:42 PST
Created attachment 188816 [details]
Use svn log -r without updating checkout
Comment 2 Ryosuke Niwa 2013-02-18 01:21:21 PST
Created attachment 188817 [details]
Merged ToT
Comment 3 Ryosuke Niwa 2013-02-18 01:35:39 PST
I'm using the new implementation now. It's MUCH faster :D
Comment 4 Ryosuke Niwa 2013-02-18 02:32:01 PST
Created attachment 188834 [details]
Also use utf-8 encoding
Comment 5 Adam Barth 2013-02-19 10:33:00 PST
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?
Comment 6 Adam Barth 2013-02-19 10:33:19 PST
Does it update it's working copy periodically?
Comment 7 Ryosuke Niwa 2013-02-19 10:36:36 PST
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.
Comment 8 Ryosuke Niwa 2013-02-19 10:37:31 PST
(In reply to comment #6)
> Does it update it's working copy periodically?

No. My plan is to have restart command do it.
Comment 9 Adam Barth 2013-02-19 12:15:59 PST
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?
Comment 10 Ryosuke Niwa 2013-02-19 14:03:31 PST
(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?
Comment 11 Adam Barth 2013-02-19 14:08:03 PST
Sure.  Those names sound fine.
Comment 12 Ryosuke Niwa 2013-04-08 23:39:37 PDT
Created attachment 197004 [details]
Fixed per comments
Comment 13 Geoffrey Garen 2013-04-08 23:41:27 PDT
Comment on attachment 197004 [details]
Fixed per comments

r=me
Comment 14 Ryosuke Niwa 2013-04-08 23:43:22 PDT
Committed r147984: <http://trac.webkit.org/changeset/147984>