This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4498

util: properly copy lrucachedict instances
ClosedPublic

Authored by indygreg on Sep 6 2018, 9:17 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGb31b01f93b11: util: properly copy lrucachedict instances
Summary

Previously, copy() only worked if the cache was full. We teach
copy() to only copy defined nodes.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Sep 6 2018, 9:17 PM
martinvonz added a subscriber: martinvonz.Sep 7 2018, 12:39 AM
martinvonz added inline comments.
mercurial/util.py
1302

Btw, why doesn't this do return self[k] instead? It seems surprising that the recency is not updated.

indygreg added inline comments.Sep 7 2018, 1:13 PM
mercurial/util.py
1302

Yes, this does feel like a bug. But it feels unrelated to the series, no?

I can certainly code up a patch easily enough. I may tack one on the end of the series...

martinvonz added inline comments.Sep 7 2018, 1:14 PM
mercurial/util.py
1302

Yes, definitely unrelated (that's what the "Btw" was supposed to mean, but I could have been clearer)

indygreg added a comment.Sep 7 2018, 1:21 PM

Added D4506 to the series to fix the bug with get().

Closed by commit rHGb31b01f93b11: util: properly copy lrucachedict instances (authored by indygreg). · Explain WhySep 12 2018, 10:52 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/util.py (10 lines)
Mtests/test-lrucachedict.py (20 lines)

Diff 10817

mercurial/util.py

# this node the oldest item. # this node the oldest item.
self._movetohead(node) self._movetohead(node)
self._head = node.next self._head = node.next
# Additional dict methods. # Additional dict methods.
def get(self, k, default=None): def get(self, k, default=None):
try: try:
return self._cache[k].value return self._cache[k].value
martinvonzUnsubmitted
Not Done

Btw, why doesn't this do return self[k] instead? It seems surprising that the recency is not updated.

martinvonz: Btw, why doesn't this do `return self[k]` instead? It seems surprising that the recency is not…
indygregAuthorUnsubmitted
Not Done

Yes, this does feel like a bug. But it feels unrelated to the series, no?

I can certainly code up a patch easily enough. I may tack one on the end of the series...

indygreg: Yes, this does feel like a bug. But it feels unrelated to the series, no? I can certainly code…
martinvonzUnsubmitted
Not Done

Yes, definitely unrelated (that's what the "Btw" was supposed to mean, but I could have been clearer)

martinvonz: Yes, definitely unrelated (that's what the "Btw" was supposed to mean, but I could have been…
except KeyError: except KeyError:
return default return default
def clear(self): def clear(self):
n = self._head n = self._head
while n.key is not _notset: while n.key is not _notset:
n.markempty() n.markempty()
n = n.next n = n.next
self._cache.clear() self._cache.clear()
def copy(self): def copy(self):
result = lrucachedict(self._capacity) result = lrucachedict(self._capacity)
# We copy entries by iterating in oldest-to-newest order so the copy
# has the correct ordering.
# Find the first non-empty entry.
n = self._head.prev n = self._head.prev
# Iterate in oldest-to-newest order, so the copy has the right ordering while n.key is _notset and n is not self._head:
n = n.prev
for i in range(len(self._cache)): for i in range(len(self._cache)):
result[n.key] = n.value result[n.key] = n.value
n = n.prev n = n.prev
return result return result
def _movetohead(self, node): def _movetohead(self, node):
"""Mark a node as the newest, making it the new head. """Mark a node as the newest, making it the new head.
When a node is accessed, it becomes the freshest entry in the LRU When a node is accessed, it becomes the freshest entry in the LRU
list, which is denoted by self._head. list, which is denoted by self._head.

tests/test-lrucachedict.py

def testcopypartial(self): def testcopypartial(self):
d = util.lrucachedict(4) d = util.lrucachedict(4)
d['a'] = 'va' d['a'] = 'va'
d['b'] = 'vb' d['b'] = 'vb'
dc = d.copy() dc = d.copy()
self.assertEqual(len(dc), 2) self.assertEqual(len(dc), 2)
# TODO this fails
return
for key in ('a', 'b'): for key in ('a', 'b'):
self.assertIn(key, dc) self.assertIn(key, dc)
self.assertEqual(dc[key], 'v%s' % key) self.assertEqual(dc[key], 'v%s' % key)
self.assertEqual(len(d), 2)
for key in ('a', 'b'):
self.assertIn(key, d)
self.assertEqual(d[key], 'v%s' % key)
d['c'] = 'vc'
del d['b']
dc = d.copy()
self.assertEqual(len(dc), 2)
for key in ('a', 'c'):
self.assertIn(key, dc)
self.assertEqual(dc[key], 'v%s' % key)
def testcopyempty(self):
d = util.lrucachedict(4)
dc = d.copy()
self.assertEqual(len(dc), 0)
def testcopyfull(self): def testcopyfull(self):
d = util.lrucachedict(4) d = util.lrucachedict(4)
d['a'] = 'va' d['a'] = 'va'
d['b'] = 'vb' d['b'] = 'vb'
d['c'] = 'vc' d['c'] = 'vc'
d['d'] = 'vd' d['d'] = 'vd'
dc = d.copy() dc = d.copy()