This is an archive of the discontinued Mercurial Phabricator instance.

branchmap: encapsulate cache updating in the map itself
ClosedPublic

Authored by mjpieters on Jan 21 2019, 1:08 PM.

Details

Summary

Rather than have a repository update the cache, move handling of cache updates
into the branchmap module, in the form of a custom mapping class.

This makes later performance improvements easier to handle too.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mjpieters created this revision.Jan 21 2019, 1:08 PM
martinvonz added inline comments.
contrib/perf.py
2297

Same here: I think this needs to be made compatible with both versions (before and after this patch)

mercurial/localrepo.py
2077

Hmm, it's much less clear now that this updates the cache. At the very least, it deserves a comment. Is the updatecache() call from __getitem__() necessary for your later patches? Sorry, I didn't quite follow.

mercurial/streamclone.py
15–16

test-check-pyflakes.t says that this is now unused

mjpieters marked an inline comment as done.Jan 26 2019, 8:45 AM
mjpieters updated this revision to Diff 13502.
mjpieters added inline comments.Jan 26 2019, 8:46 AM
contrib/perf.py
2297

I'll wait for confirmation; see the other patch.

mercurial/localrepo.py
2077

Yes, the updating will change in a later, as yet to be submitted patch.

But why should it be clear here that there is a cache that is kept up-to-date on access to the branchmap? That's a responsibility of the cache, not of whatever accesses a branchmap. The cache is an implementation detail of branchmap, and should really not bleed out into code that merely consumes the map.

martinvonz added inline comments.Jan 26 2019, 9:45 AM
mercurial/localrepo.py
2077

Fair enough, it doesn't have to be clear that it updates the cache, but it has to be clear that it does *something*. It currently looks like a statement that can be removed, but I assume it cannot.

mjpieters updated this revision to Diff 13543.Jan 28 2019, 1:05 PM
mjpieters marked 2 inline comments as done.Jan 28 2019, 1:05 PM
pulkit added a subscriber: pulkit.Feb 7 2019, 3:29 PM

This looks good to me. @martinvonz do you have any concerns?

In D5638#86131, @pulkit wrote:

This looks good to me. @martinvonz do you have any concerns?

Yes, looks good. Sorry, I thought this had already been queued.

pulkit accepted this revision.Feb 8 2019, 7:04 AM
This revision was automatically updated to reflect the committed changes.