D4366: treemanifest: introduce lazy loading of subdirs
spectral (Kyle Lippincott)
phabricator at mercurial-scm.org
Fri Aug 24 18:36:17 EDT 2018
spectral planned changes to this revision.
spectral added a comment.
In https://phab.mercurial-scm.org/D4366#67144, @indygreg wrote:
> I'm not sure how I feel about so many methods having the `if dir in self._lazydirs: self._loadlazy(dir)` pattern.
>
> On one hand, action at a distance when it involves caching can be dangerous. And doing the lookup inline will avoid a Python function call.
>
> On the other, it seems very redundant.
>
> Overall I'm OK with the patch. I just feel like using a `collections.defaultdict` or implementing `__missing__` to automagically resolve missing keys might work out better. Do you think this is a reasonable request?
I think it's a reasonable request, but I'm not sure it's feasible with how collections.defaultdict/__missing__ work. They are *only* called by __getitem__. We'd need a separate dict subclass for _dirs that handles __contains__ as well, at the very least, for all the cases of `if dir not in self._dirs: return <something>`
I'm playing around with such a thing, it's not difficult but I think might be more awkward than helpful. Even if that doesn't bear fruit, while implementing it I noticed something: I'm not actually populating self._lazydirs in this patch, I appear to have lost it during split/merge/whatever. Oops. I have to hope that the numbers I have in the commit description were before I lost this, since otherwise it's completely incomprehensible that this patch would cause a performance benefit (well, maybe not, if dead code can cause a performance loss, why not the other way around?)
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D4366
To: spectral, #hg-reviewers
Cc: indygreg, mercurial-devel
More information about the Mercurial-devel
mailing list