[PATCH] hgweb: build the "entries" list directly in filelog command

Pierre-Yves David pierre-yves.david at ens-lyon.org
Mon Jan 16 12:22:56 EST 2017



On 01/16/2017 11:59 AM, Denis Laxalde wrote:
> Martin von Zweigbergk via Mercurial-devel a écrit :
>>
>>
>> On Sat, Jan 14, 2017, 23:52 Pierre-Yves David
>> <pierre-yves.david at ens-lyon.org <mailto:pierre-yves.david at ens-lyon.org>>
>> wrote:
>>
>>
>>
>>     On 01/14/2017 06:14 AM, Gregory Szorc wrote:
>>     > On Fri, Jan 13, 2017 at 8:08 AM, Pierre-Yves David
>>     > <pierre-yves.david at ens-lyon.org
>>     <mailto:pierre-yves.david at ens-lyon.org>
>>     <mailto:pierre-yves.david at ens-lyon.org
>>     <mailto:pierre-yves.david at ens-lyon.org>>>
>>     > wrote:
>>     >
>>     >
>>     >
>>     >     On 01/13/2017 04:31 PM, Denis Laxalde wrote:
>>     >
>>     >         # HG changeset patch
>>     >         # User Denis Laxalde <denis.laxalde at logilab.fr
>>     <mailto:denis.laxalde at logilab.fr>
>>     >         <mailto:denis.laxalde at logilab.fr
>>     <mailto:denis.laxalde at logilab.fr>>>
>>     >         # Date 1484299345 -3600
>>     >         #      Fri Jan 13 10:22:25 2017 +0100
>>     >         # Node ID ad41cc2eb799156b0de2dd71adeed88ec42a98d9
>>     >         # Parent  e882c7bb5a0ba2589a44108c9a87b300a13e08df
>>     >         hgweb: build the "entries" list directly in filelog command
>>     >
>>     >         There's no apparent reason to have this "entries" generator
>>     >         function that
>>     >         builds a list and then yields its elements in reverse
>>     order and
>>     >         which is only
>>     >         called to build the "entries" list. So just build the list
>>     >         directly, in
>>     >         reverse order.
>>     >
>>     >
>>     >     revlog are usually read from 0 to max for performance reason
>> (data
>>     >     in N+1 might rely on data on N and last access might be
>> cached).
>>     >     This might explain why the list was build in one direction and
>>     then
>>     >     reversed.
>>     >     Can you double check if such mechanism is in play in this case?
>>     >
>>     >
>>     > Your observation about desiring to iterate revlogs from 0 to max
>>     is spot
>>     > on and this patch would normally be wrong. However, in this code
>>     `start
>>     >> end` (as can be seen in the context just above the first changed
>>     > line), so we're already iterating in the suboptimal direction. This
>>     > patch actually fixes that by throwing in a reversed() of a
>> descending
>>     > fctx.filelog().revs()!
>>
>>
>> These are the lines from the context:
>>
>> count = fctx.filerev() + 1
>> start = max(0, fctx.filerev() - revcount + 1) # first rev on this page
>> end = min(count, start + revcount) # last rev on this page
>>
>> Let's say count=100, revcount=10. Then start=90 and end=100. Since you
>> two (Greg and Pierre-Yves) agree that start > end, I suspect I'm just
>> missing something, but I couldn't figure out what.
>
>
> That's also my impression. Previously revisions were iterated over in
> ascending order, now they are in descending order.

I think Martin and Denis are right. I just got fooled by the start = 
max(…); stop = min(…) (that is embarrassing). Thanks for spotting that.

> If this causes performance regressions (I could not observe any), I can
> send a patch restoring iteration in ascending order.

Can you provide use with some detail about the performance testing you 
did? If possible with associated number?

In all cases it is probably worthwhile to keep ascending order here as I 
don't see any reason it would hurt.

Cheers,

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list