Page MenuHomePhabricator

branchmap: encapsulate cache updating in the map itself

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



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

rHG Mercurial
Automatic diff as part of commit; lint not applicable.
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.

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


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.


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

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


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

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.