[PATCH 2 of 6 v2] util: add an elapsed time wrapper

Bryan O'Sullivan bos at serpentine.com
Thu Feb 2 19:54:43 EST 2017


On Thu, Feb 2, 2017 at 11:32 AM, Bryan O'Sullivan <bos at serpentine.com>
wrote:

>
> To be honest, this seems like a heavily over-engineered approach to me.
>

I don't like giving such vague feedback, but I was hopping off the shuttle
this morning and couldn't write anything more concrete at the time. Sorry
about that.

I also feel somewhat apologetic about seagulling in abruptly with a
critique after months and months of silence, but ... hi?

I wanted to substantiate my sense of unease with this patchset. Here's what
I came up with.

First, you'll want to take a look at my different approach, which I just
sent out a moment ago. (It's not cooked, and I forgot to add the "RFC" tag,
but you get the idea.)

Next, performance. Here's a little benchmark: http://pastebin.com/f9eFEWiq

On my laptop, base Mercurial runs it in 0.15 seconds.
With performance logging enabled, your patchset takes 0.36 seconds.
And my patchset takes 0.16, with no need to enable or disable anything.

To be clear, the performance numbers should not be persuasive, because a
typical invocation of Mercurial will call ui.write maybe 1% as often as the
benchmark does. But what they do is give me another angle from which to
look at "this doesn't feel right to me".

Your patch adds a bunch of allocation and indirection on what for this
benchmark is a fast path, which is why it appears so costly.

Abstractions in Python have a heavy runtime cost and of course the usual
"now I have an abstraction to understand" overhead, and so you have to be
particularly judicious in creating and using them.

The above approach of indirection via a wrapper object doesn't meet my
personal "abstraction that pays for the cost of understanding and using it"
bar, especially as the alternative approach in my patchset is both simple
to understand and has almost no overhead.

Also, the way that you're conditionally recording time used in both this
and later patches (for crecord and extdiff) strikes me as unnecessarily
complex. My patches demonstrate that doing so unconditionally is simpler
and has inconsequential overhead.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170202/e369f2fb/attachment.html>


More information about the Mercurial-devel mailing list