<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 7, 2019 at 9:54 AM Gregory Szorc <<a href="mailto:gregory.szorc@gmail.com">gregory.szorc@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 7, 2019 at 4:23 AM Pierre-Yves David <<a href="mailto:pierre-yves.david@ens-lyon.org" target="_blank">pierre-yves.david@ens-lyon.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
On 3/6/19 9:35 PM, Yuya Nishihara wrote:<br>
> On Tue, 05 Mar 2019 18:39:15 +0100, Pierre-Yves David wrote:<br>
>> # HG changeset patch<br>
>> # User Pierre-Yves David <<a href="mailto:pierre-yves.david@octobus.net" target="_blank">pierre-yves.david@octobus.net</a>><br>
>> # Date 1551311787 -3600<br>
>> # Thu Feb 28 00:56:27 2019 +0100<br>
>> # Node ID ffd98d208aa7f92e13bf233b6d752cd2d292ebbe<br>
>> # Parent 82035c1d714f8f3911632ea1271002745fc620f4<br>
>> # EXP-Topic discovery-speedup<br>
>> # Available At <a href="https://bitbucket.org/octobus/mercurial-devel/" rel="noreferrer" target="_blank">https://bitbucket.org/octobus/mercurial-devel/</a><br>
>> # hg pull <a href="https://bitbucket.org/octobus/mercurial-devel/" rel="noreferrer" target="_blank">https://bitbucket.org/octobus/mercurial-devel/</a> -r ffd98d208aa7<br>
>> discovery: use a lower level but faster way to retrieve parents<br>
> <br>
>> diff --git a/mercurial/setdiscovery.py b/mercurial/setdiscovery.py<br>
>> --- a/mercurial/setdiscovery.py<br>
>> +++ b/mercurial/setdiscovery.py<br>
>> @@ -165,6 +165,12 @@ class partialdiscovery(object):<br>
>> # common.bases and all its ancestors<br>
>> return self._common.basesheads()<br>
>> <br>
>> + def _parentsgetter(self):<br>
>> + getrev = self._repo.changelog.index.__getitem__<br>
>> + def getparents(r):<br>
>> + return getrev(r)[5:6]<br>
>> + return getparents<br>
> <br>
> Queued, but it's probably better to use revlog._uncheckedparentrevs() than<br>
> peeking a raw index entry. Can you send a follow up?<br>
<br>
Unfortunately, the overhead of _uncheckedparentrevs is still quite <br>
significant. (Attribute access and exception handling)<br>
<br>
before: 66.831393 seconds (median of 3)<br>
my-change: 49.357107 seconds (median of 3)<br>
_uncheckedparentrevs: 58.941629 seconds (median of 3)<br></blockquote><div><br></div><div>These timings are bonkers and I am quite surprised that attribute access and exception handling is adding that much overhead! Are we sure there isn't a `repo.changelog` in a tight loop? Or maybe we're being grossly inefficient about DAG traversals and missing some caching opportunities?<br></div><div><br></div><div>I agree with Yuya that accessing the raw index entries is undesirable. This extremely low-level interface will almost certainly not be supported when we establish a formal storage interface for the changelog. If there are performance concerns here, we *really* need an inline comment to that effect. And it would be great if it pointed to a perf* command that could help measure.</div><div><br></div><div>I'll accept this. But without an inline comment and a documented way to reproduce the performance problem, there's a very good chance we regress this optimization when establishing a formal interface for changelog. Please follow up to help mitigate this possibility.</div></div></div></blockquote><div><br></div><div>... and the very next changeset in this series (c98420914c1 on committed) says "on our private pathological case, this speeds things up from about 53 seconds to about 7.5 seconds." And 5baf06d2bb4 drops to 6.3s. So it was indeed a very inefficient algorithm plus lack of caching contributing to extreme amounts of overhead.<br></div><div><br></div><div>Is this micro optimization needed by the end state of the series? If not, perhaps it should be reverted or my original comment asking for a follow-up should be ignored. Whatever the outcome, a perf* command or more details on how to simulate this performance problem (such as the number of changesets/heads involved) would really help. Even 6.3s for discovery feels a bit steep and if we're serious about scaling, we need to be presented with esoteric use cases like your private repo to consider alternate ways of discovery.<br></div></div></div></div></div></div>