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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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

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
2390

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.