Switch to the good LRU implementation.

Switch to a better LRU implementation.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127266

[M-A Ruel, 2012-03-16 13:02:05]

lgtm with a few nits, thanks!

https://chromiumcodereview.appspot.com/9703108/diff/1/third_party/buildbot_8_...
File third_party/buildbot_8_4p1/buildbot/status/builder.py (right):

https://chromiumcodereview.appspot.com/9703108/diff/1/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/status/builder.py:89: #self.buildCache =
weakref.WeakValueDictionary()
I'd simply remove the old lines instead of commenting them.

https://chromiumcodereview.appspot.com/9703108/diff/1/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/status/builder.py:243: for b in
self.currentBuilds:
if any(b.number == number for b in self.currentBuilds):

[cmp, 2012-03-16 17:40:51]

lgtm with maruel's comments

https://chromiumcodereview.appspot.com/9703108/diff/1/third_party/buildbot_8_...
File third_party/buildbot_8_4p1/buildbot/status/builder.py (right):

https://chromiumcodereview.appspot.com/9703108/diff/1/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/status/builder.py:89: #self.buildCache =
weakref.WeakValueDictionary()
On 2012/03/16 13:02:06, Marc-Antoine Ruel wrote:
> I'd simply remove the old lines instead of commenting them.

Agree.

[szager, 2012-03-16 20:52:04]

Added SyncLRUCache, and use it in BuilderStatus.

https://chromiumcodereview.appspot.com/9703108/diff/1/third_party/buildbot_8_...
File third_party/buildbot_8_4p1/buildbot/status/builder.py (right):

https://chromiumcodereview.appspot.com/9703108/diff/1/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/status/builder.py:89: #self.buildCache =
weakref.WeakValueDictionary()
On 2012/03/16 17:40:51, cmp wrote:
> On 2012/03/16 13:02:06, Marc-Antoine Ruel wrote:
> > I'd simply remove the old lines instead of commenting them.
> 
> Agree.

Done.

https://chromiumcodereview.appspot.com/9703108/diff/1/third_party/buildbot_8_...
third_party/buildbot_8_4p1/buildbot/status/builder.py:243: for b in
self.currentBuilds:
On 2012/03/16 13:02:06, Marc-Antoine Ruel wrote:
> if any(b.number == number for b in self.currentBuilds):

I don't like that because it requires two linear searches through
self.currentBuilds -- once for 'any', and then again to return the value.

[szager, 2012-03-16 21:50:08]

Small bug fix, and added a unit test.

[szager, 2012-03-16 21:51:03]

Would you mind reviewing this again?  It didn't have SyncLRUCache before.

Thanks,

Stefan

On Fri, Mar 16, 2012 at 2:50 PM,  <szager@google.com> wrote:
> Small bug fix, and added a unit test.
>
> https://chromiumcodereview.appspot.com/9703108/

[cmp, 2012-03-16 21:57:08]

https://chromiumcodereview.appspot.com/9703108/diff/10001/third_party/buildbo...
File third_party/buildbot_8_4p1/buildbot/test/unit/test_status_builder_cache.py
(right):

https://chromiumcodereview.appspot.com/9703108/diff/10001/third_party/buildbo...
third_party/buildbot_8_4p1/buildbot/test/unit/test_status_builder_cache.py:28: #
Ackwardly, Status sets this member variable.
Ackwardly -> Awkwardly

https://chromiumcodereview.appspot.com/9703108/diff/10001/third_party/buildbo...
third_party/buildbot_8_4p1/buildbot/test/unit/test_status_builder_cache.py:60:
'propval%d' % build.number)
can you add a test to verify that the cache's hits are increasing with each
call?

[szager, 2012-03-16 22:02:24]

https://chromiumcodereview.appspot.com/9703108/diff/10001/third_party/buildbo...
File third_party/buildbot_8_4p1/buildbot/test/unit/test_status_builder_cache.py
(right):

https://chromiumcodereview.appspot.com/9703108/diff/10001/third_party/buildbo...
third_party/buildbot_8_4p1/buildbot/test/unit/test_status_builder_cache.py:28: #
Ackwardly, Status sets this member variable.
On 2012/03/16 21:57:08, cmp wrote:
> Ackwardly -> Awkwardly

Done.

https://chromiumcodereview.appspot.com/9703108/diff/10001/third_party/buildbo...
third_party/buildbot_8_4p1/buildbot/test/unit/test_status_builder_cache.py:60:
'propval%d' % build.number)
On 2012/03/16 21:57:08, cmp wrote:
> can you add a test to verify that the cache's hits are increasing with each
> call?

Done.

[cmp, 2012-03-16 22:03:26]

lgtm

[cmp, 2012-03-16 22:04:55]

FYI, I scanned AsyncLRUCache to look for any other instances of deferreds and
confirmed that they were all in get().  There were style nits I found too, but
they all matched the Buildbot style you had copied from.

[commit-bot: I haz the power, 2012-03-16 22:10:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szager@google.com/9703108/8005

[commit-bot: I haz the power, 2012-03-16 22:12:49]

Change committed as 127266