[Commented On] D9573: branchmap: update rev-branch-cache incrementally

[marmoute (Pierre-Yves David)]

marmoute added inline comments.

INLINE COMMENTS

> joerg.sonnenberger wrote in branchmap.py:660-673
> The extra logic is essentially what `changelog.branchinfo` does. D9603 <https://phab.mercurial-scm.org/D9603> refactors that to put it into `revbranchcache` as the only consumer in core. I've kept it for the moment since topics interacts with this code and would break.

My main concerns is responsibility and implementation details leaking into a simple cache. Could we have the extra-extraction done in the register_changeset method itself ? or probably better, putting them on changelogrevision since it is the one responsible for the details of the data access.

> joerg.sonnenberger wrote in branchmap.py:660-673
> I think redundant keys are an interface bug. The `node` was available for all, so at least `onchangelog` in `changegroup.py` would have to do a lookup anyway. For the `commit` caller it would mean shuffling things around a bit.
> 
> I'll check the extra logic here again, it's been inconsistent across the code base. This is a bit of a left over from earlier iterations, but it can likely be made tighter and simpler now.

Both `changegroup` and `commit` application should have a revision number handy (that just the size of the revlog before creating the changeset). Revision number are much more efficient that nodeid overall. I would much prefer this kind of low level API to standardize on using them.

> joerg.sonnenberger wrote in branchmap.py:692-694
> Extending by requiredsize will normally double the size, except when it was sparse before. In that case the gap will count toward to extension, which seems a reasonable compromise.

I get it now. I don't know if it me being tired of the code being a bit too sneaky. but it might be worth having some explicit arithmetic or a clarification comment to highlight that the operation result in a array that is "current_size + currently needed size" leading to (about) a ×2 size increase.

> joerg.sonnenberger wrote in repository.py:1644-1650
> I'm not attached to the name. The other concern was if it should be function on the module scope to allow easier monkey patching. Not sure if that question warrants splitting the change.

Wrapping of repository level method is a common operation in extension. That is probably a good place to put it. (And also a place because they are too many method on localrepo… but these one is not a too bad fit in my opinion.)

The motivation to split the change is also about making the first half easier to land sooner, and to avoid mixing discussion on two different topic. That is not a requirement, but it might help.

> joerg.sonnenberger wrote in repository.py:1647-1648
> I tried adding a cache for the last `changelogrevision`, but it turned out to be more expensive than shuffling things around a bit here. See the remark about `readfiles` earlier, which is the primarily time sink right now.

could we cache the result of readfiles ? to reuse it at that time ?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D9573/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D9573

To: joerg.sonnenberger, #hg-reviewers, marmoute
Cc: marmoute, pulkit, mercurial-patches
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-patches/attachments/20210114/982d5822/attachment-0002.html>