On Thu, Feb 2, 2017 at 11:32 AM, Bryan O'Sullivan <b...@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.
_______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel