Patchwork [1,of,8] perf: introduce safeattrsetter to replace direct attribute assignment

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Aug. 8, 2016, 9:59 a.m.
Message ID <e5f93a8eb06c9b189c4f.1470650380@juju>
Download mbox | patch
Permalink /patch/16195/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Katsunori FUJIWARA - Aug. 8, 2016, 9:59 a.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1470649697 -32400
#      Mon Aug 08 18:48:17 2016 +0900
# Node ID e5f93a8eb06c9b189c4f2460a66fe8f2d7713655
# Parent  84a8de5ac901c00f4efec1ec2e0be24ebd970fd8
perf: introduce safeattrsetter to replace direct attribute assignment

Referring not-existing attribute immediately causes failure, but
assigning a value to such attribute doesn't.

For example, perf.py has code paths below, which assign a value to
not-existing attribute. This causes incorrect performance measurement,
but these code paths are executed successfully.

  - "repo._tags = None" in perftags()
    recent Mercurial has tags cache information in repo._tagscache

  - "branchmap.write = lambda repo: None" in perfbranchmap()
    branchmap cache is written out by branchcache.write() in branchmap.py

"util.safehasattr() before assignment" can avoid this issue, but might
increase mistake at "copy & paste" attribute name or so.

To centralize (1) examining existence of, (2) assigning a value to,
and (3) restoring an old value of an attribute, this patch introduces
safeattrsetter(). This is used to replace direct attribute assignment
in subsequent patches.

To avoid examining existence of attribute at each repetition, this
patch makes safeattrsetter() return the function, which actually
assigns a value to an attribute.

Encapsulation of restoring is needed to completely remove direct
attribute assignment from perf.py, even though restoring isn't needed
so often.
Yuya Nishihara - Aug. 8, 2016, 2:20 p.m.
On Mon, 08 Aug 2016 18:59:40 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1470649697 -32400
> #      Mon Aug 08 18:48:17 2016 +0900
> # Node ID e5f93a8eb06c9b189c4f2460a66fe8f2d7713655
> # Parent  84a8de5ac901c00f4efec1ec2e0be24ebd970fd8
> perf: introduce safeattrsetter to replace direct attribute assignment

> To avoid examining existence of attribute at each repetition, this
> patch makes safeattrsetter() return the function, which actually
> assigns a value to an attribute.

[snip]

> +def safeattrsetter(obj, name, ignoremissing=False):
> +    """Ensure that 'obj' has 'name' attribute before execution of setattr
> +
> +    This function is aborted, if 'obj' doesn't have 'name' attribute
> +    at runtime. This avoids overlooking removal of an attribute, which
> +    breaks assumption of performance measurement, in the future.
> +
> +    This function returns the function to assign a value to 'name'
> +    attribute. This "setter" function returns the function to restore
> +    'name' attribute to old one.
> +
> +    If 'ignoremissing' is true, missing 'name' attribute doesn't cause
> +    abortion, and this function returns None. This is useful to
> +    examine an attribute, which isn't ensured in all Mercurial
> +    versions.
> +    """
> +    if not util.safehasattr(obj, name):
> +        if ignoremissing:
> +            return None
> +        raise error.Abort(("missing attribute %s of %s might break assumption"
> +                           " of performance measurement") % (name, obj))
> +
> +    def setter(newvalue):
> +        oldvalue = getattr(obj, name)

If you care for the cost of safehasattr() (that could load a propertycache?),
getattr() should be removed from the perf loop as well.

> +        setattr(obj, name, newvalue)
> +        def restorer():
> +            setattr(obj, name, oldvalue)
> +        return restorer

Maybe this could be a class to avoid long closure chain.

  s = safeattrsetter(obj, name)  # remember current obj.name (=v0)
  s.set(v1)
  s.set(v2)
  s.restore()  # restore v0, not v1
Katsunori FUJIWARA - Aug. 8, 2016, 2:59 p.m.
At Mon, 8 Aug 2016 23:20:04 +0900,
Yuya Nishihara wrote:
> 
> On Mon, 08 Aug 2016 18:59:40 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1470649697 -32400
> > #      Mon Aug 08 18:48:17 2016 +0900
> > # Node ID e5f93a8eb06c9b189c4f2460a66fe8f2d7713655
> > # Parent  84a8de5ac901c00f4efec1ec2e0be24ebd970fd8
> > perf: introduce safeattrsetter to replace direct attribute assignment
> 
> > To avoid examining existence of attribute at each repetition, this
> > patch makes safeattrsetter() return the function, which actually
> > assigns a value to an attribute.
> 
> [snip]
> 
> > +def safeattrsetter(obj, name, ignoremissing=False):
> > +    """Ensure that 'obj' has 'name' attribute before execution of setattr
> > +
> > +    This function is aborted, if 'obj' doesn't have 'name' attribute
> > +    at runtime. This avoids overlooking removal of an attribute, which
> > +    breaks assumption of performance measurement, in the future.
> > +
> > +    This function returns the function to assign a value to 'name'
> > +    attribute. This "setter" function returns the function to restore
> > +    'name' attribute to old one.
> > +
> > +    If 'ignoremissing' is true, missing 'name' attribute doesn't cause
> > +    abortion, and this function returns None. This is useful to
> > +    examine an attribute, which isn't ensured in all Mercurial
> > +    versions.
> > +    """
> > +    if not util.safehasattr(obj, name):
> > +        if ignoremissing:
> > +            return None
> > +        raise error.Abort(("missing attribute %s of %s might break assumption"
> > +                           " of performance measurement") % (name, obj))
> > +
> > +    def setter(newvalue):
> > +        oldvalue = getattr(obj, name)
> 
> If you care for the cost of safehasattr() (that could load a propertycache?),
> getattr() should be removed from the perf loop as well.
> 
> > +        setattr(obj, name, newvalue)
> > +        def restorer():
> > +            setattr(obj, name, oldvalue)
> > +        return restorer
> 
> Maybe this could be a class to avoid long closure chain.
> 
>   s = safeattrsetter(obj, name)  # remember current obj.name (=v0)
>   s.set(v1)
>   s.set(v2)
>   s.restore()  # restore v0, not v1

That looks better. I'll post revised one, thank you.

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp

Patch

diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -181,6 +181,41 @@  def _timer(fm, func, title=None):
     fm.write('count',  ' (best of %d)', count)
     fm.plain('\n')
 
+# utilities for historical portability
+
+def safeattrsetter(obj, name, ignoremissing=False):
+    """Ensure that 'obj' has 'name' attribute before execution of setattr
+
+    This function is aborted, if 'obj' doesn't have 'name' attribute
+    at runtime. This avoids overlooking removal of an attribute, which
+    breaks assumption of performance measurement, in the future.
+
+    This function returns the function to assign a value to 'name'
+    attribute. This "setter" function returns the function to restore
+    'name' attribute to old one.
+
+    If 'ignoremissing' is true, missing 'name' attribute doesn't cause
+    abortion, and this function returns None. This is useful to
+    examine an attribute, which isn't ensured in all Mercurial
+    versions.
+    """
+    if not util.safehasattr(obj, name):
+        if ignoremissing:
+            return None
+        raise error.Abort(("missing attribute %s of %s might break assumption"
+                           " of performance measurement") % (name, obj))
+
+    def setter(newvalue):
+        oldvalue = getattr(obj, name)
+        setattr(obj, name, newvalue)
+        def restorer():
+            setattr(obj, name, oldvalue)
+        return restorer
+
+    return setter
+
+# perf commands
+
 @command('perfwalk', formatteropts)
 def perfwalk(ui, repo, *pats, **opts):
     timer, fm = gettimer(ui, opts)