| Submitter | Katsunori FUJIWARA |
|---|---|
| Date | June 10, 2016, 7:20 p.m. |
| Message ID | <ab257e0c53440b80edd5.1465586420@feefifofum> |
| Download | mbox | patch |
| Permalink | /patch/15464/ |
| State | Changes Requested |
| Headers | show |
Comments
The code itself seems mostly good to me (I need a second pass) but I'm a bit confused about why we want to make perf.py backward compatible? Old version of Mercurial can use the old version of the perf extension, isn't that enough? I'm curious about your use case here. On 06/10/2016 09:20 PM, FUJIWARA Katsunori wrote: > # HG changeset patch > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp> > # Date 1465585986 -32400 > # Sat Jun 11 04:13:06 2016 +0900 > # Node ID ab257e0c53440b80edd52e312f6ac8859e6c65b5 > # Parent b5487e8e3142fc650b5fa3d0fbe3e8ff4e24da3e > perf: define formatter locally if Mercurial is earlier than 2.2 > > Before this patch, using ui.formatter() prevents perf.py from > measuring performance with Mercurial earlier than 2.2 (or > ae5f92e154d3), because ui.formatter() isn't available in such > Mercurial, even though there are some code paths for Mercurial earlier > than 2.2. > > For example, setting "_prereadsize" attribute in perfindex() and > perfnodelookup() is effective only with hg earlier than 1.8 (or > 61c9bc3da402). > > This patch defines formatter class locally, and use it instead of the > value returned by ui.formatter(), if Mercurial is earlier than 2.2 at > runtime. > > In this case, we don't need to think about -T/--template option for > formatter, because previous patch made -T/--template disabled for > perf.py with Mercurial earlier than 3.2 (or 7a7eed5176a4). > > diff --git a/contrib/perf.py b/contrib/perf.py > --- a/contrib/perf.py > +++ b/contrib/perf.py > @@ -94,7 +94,36 @@ def gettimer(ui, opts=None): > ui = ui.copy() > ui.fout = ui.ferr > # get a formatter > - fm = ui.formatter('perf', opts) > + if util.safehasattr(ui, 'formatter'): > + fm = ui.formatter('perf', opts) > + else: > + # ui.formatter has been available since 2.2 (or ae5f92e154d3) > + from mercurial import node > + class defaultformatter(object): > + """Minimized composition of baseformatter and plainformatter > + """ > + def __init__(self, ui, topic, opts): > + self._ui = ui > + if ui.debugflag: > + self.hexfunc = node.hex > + else: > + self.hexfunc = node.short > + def __nonzero__(self): > + return False > + def startitem(self): > + pass > + def data(self, **data): > + pass > + def write(self, fields, deftext, *fielddata, **opts): > + self._ui.write(deftext % fielddata, **opts) > + def condwrite(self, cond, fields, deftext, *fielddata, **opts): > + if cond: > + self._ui.write(deftext % fielddata, **opts) > + def plain(self, text, **opts): > + self._ui.write(text, **opts) > + def end(self): > + pass > + fm = defaultformatter(ui, 'perf', opts) > # stub function, runs code only once instead of in a loop > # experimental config: perf.stub > if ui.configbool("perf", "stub"): > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel >
On Fri, 2016-06-17 at 19:04 +0200, Pierre-Yves David wrote: > The code itself seems mostly good to me (I need a second pass) but I'm a > bit confused about why we want to make perf.py backward compatible? Old > version of Mercurial can use the old version of the perf extension, > isn't that enough? I'm curious about your use case here. When you add new perf tests, you want to run them against historic hg. I hit this regularly. -- Mathematics is the supreme nostalgia of our time.
On 06/17/2016 08:59 PM, Matt Mackall wrote: > On Fri, 2016-06-17 at 19:04 +0200, Pierre-Yves David wrote: >> The code itself seems mostly good to me (I need a second pass) but I'm a >> bit confused about why we want to make perf.py backward compatible? Old >> version of Mercurial can use the old version of the perf extension, >> isn't that enough? I'm curious about your use case here. > > When you add new perf tests, you want to run them against historic hg. I hit > this regularly. This quest seems a bit doomed to me as perf.py is likely to keep evolving alongside the code base of Mercurial, breaking BC and using new feature. Especially now that there is some people working on improving our performance tracking. The way I usually handle this by applying a small patch with the new perf thing when doing the initial comparision (same as I do with regression tests) That said, I've nothing against this series if it help a bit, but I think we should document (near the compatibility hack) the motive better so that future contributor understant what is going on here.
At Sat, 18 Jun 2016 03:29:01 +0200, Pierre-Yves David wrote: > > On 06/17/2016 08:59 PM, Matt Mackall wrote: > > On Fri, 2016-06-17 at 19:04 +0200, Pierre-Yves David wrote: > >> The code itself seems mostly good to me (I need a second pass) but I'm a > >> bit confused about why we want to make perf.py backward compatible? Old > >> version of Mercurial can use the old version of the perf extension, > >> isn't that enough? I'm curious about your use case here. > > > > When you add new perf tests, you want to run them against historic hg. I hit > > this regularly. > > This quest seems a bit doomed to me as perf.py is likely to keep > evolving alongside the code base of Mercurial, breaking BC and using new > feature. Especially now that there is some people working on improving > our performance tracking. > > The way I usually handle this by applying a small patch with the new > perf thing when doing the initial comparision (same as I do with > regression tests) > > That said, I've nothing against this series if it help a bit, but I > think we should document (near the compatibility hack) the motive better > so that future contributor understant what is going on here. I think: we have to: - make perf.py loadable with as old version hg as possible - make historical perf tests work with as old version hg as possible we don't have to: - make newer perf tests work with old version hg (of course, it is good, if a newer test works with old hg) therefore, even if perf.py itself can be enabled with an old hg, it isn't ensured that all perf tests are executable. As far as I confirm, we can make recent perf.py loadable with Mercurial 1.1 or so by this series and some additional patches. > -- > Pierre-Yves David > ---------------------------------------------------------------------- [FUJIWARA Katsunori] foozy@lares.dti.ne.jp
On 06/18/2016 06:28 PM, FUJIWARA Katsunori wrote: > At Sat, 18 Jun 2016 03:29:01 +0200, > Pierre-Yves David wrote: >> >> On 06/17/2016 08:59 PM, Matt Mackall wrote: >>> On Fri, 2016-06-17 at 19:04 +0200, Pierre-Yves David wrote: >>>> The code itself seems mostly good to me (I need a second pass) but I'm a >>>> bit confused about why we want to make perf.py backward compatible? Old >>>> version of Mercurial can use the old version of the perf extension, >>>> isn't that enough? I'm curious about your use case here. >>> >>> When you add new perf tests, you want to run them against historic hg. I hit >>> this regularly. >> >> This quest seems a bit doomed to me as perf.py is likely to keep >> evolving alongside the code base of Mercurial, breaking BC and using new >> feature. Especially now that there is some people working on improving >> our performance tracking. >> >> The way I usually handle this by applying a small patch with the new >> perf thing when doing the initial comparision (same as I do with >> regression tests) >> >> That said, I've nothing against this series if it help a bit, but I >> think we should document (near the compatibility hack) the motive better >> so that future contributor understant what is going on here. > > I think: > > we have to: > > - make perf.py loadable with as old version hg as possible > > - make historical perf tests work with as old version hg as possible > > we don't have to: > > - make newer perf tests work with old version hg > (of course, it is good, if a newer test works with old hg) > > therefore, even if perf.py itself can be enabled with an old hg, > it isn't ensured that all perf tests are executable. > > As far as I confirm, we can make recent perf.py loadable with > Mercurial 1.1 or so by this series and some additional patches. (sorry for the delay) Okay, can you send a V2 of this series with extra comment about the intend and boundary of the compatibility effort? Cheers,
Patch
diff --git a/contrib/perf.py b/contrib/perf.py --- a/contrib/perf.py +++ b/contrib/perf.py @@ -94,7 +94,36 @@ def gettimer(ui, opts=None): ui = ui.copy() ui.fout = ui.ferr # get a formatter - fm = ui.formatter('perf', opts) + if util.safehasattr(ui, 'formatter'): + fm = ui.formatter('perf', opts) + else: + # ui.formatter has been available since 2.2 (or ae5f92e154d3) + from mercurial import node + class defaultformatter(object): + """Minimized composition of baseformatter and plainformatter + """ + def __init__(self, ui, topic, opts): + self._ui = ui + if ui.debugflag: + self.hexfunc = node.hex + else: + self.hexfunc = node.short + def __nonzero__(self): + return False + def startitem(self): + pass + def data(self, **data): + pass + def write(self, fields, deftext, *fielddata, **opts): + self._ui.write(deftext % fielddata, **opts) + def condwrite(self, cond, fields, deftext, *fielddata, **opts): + if cond: + self._ui.write(deftext % fielddata, **opts) + def plain(self, text, **opts): + self._ui.write(text, **opts) + def end(self): + pass + fm = defaultformatter(ui, 'perf', opts) # stub function, runs code only once instead of in a loop # experimental config: perf.stub if ui.configbool("perf", "stub"):