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

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Oct. 8, 2016, 4:18 p.m.
Message ID <a03b967913dd881f750a.1475943501@feefifofum>
Download mbox | patch
Permalink /patch/16942/
State Accepted
Headers show

Comments

Katsunori FUJIWARA - Oct. 8, 2016, 4:18 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1475942596 -32400
#      Sun Oct 09 01:03:16 2016 +0900
# Node ID a03b967913dd881f750a5848f567d1e0a1d0e7ea
# Parent  87b8e40eb8125d5ddc848d972b117989346057dd
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 to the attribute, this patch introduces
safeattrsetter(). This is used to replace direct attribute assignment
in subsequent patches.

Encapsulation of restoring is needed to completely remove direct
attribute assignment from perf.py, even though restoring isn't needed
so often.
Yuya Nishihara - Oct. 13, 2016, 2:50 p.m.
On Sun, 09 Oct 2016 01:18:21 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1475942596 -32400
> #      Sun Oct 09 01:03:16 2016 +0900
> # Node ID a03b967913dd881f750a5848f567d1e0a1d0e7ea
> # Parent  87b8e40eb8125d5ddc848d972b117989346057dd
> perf: introduce safeattrsetter to replace direct attribute assignment

These all look good to me. Queued, many thanks.

> +def safeattrsetter(obj, name, ignoremissing=False):
> +    """Ensure that 'obj' has 'name' attribute before subsequent 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 object to (1) assign a new value, and
> +    (2) restore an original value to the attribute.
> +
> +    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))
> +
> +    origvalue = getattr(obj, name)
> +    class attrutil(object):
> +        def set(self, newvalue):
> +            setattr(obj, name, newvalue)
> +        def restore(self):
> +            setattr(obj, name, origvalue)
> +
> +    return attrutil()

This could be a class sefeattrsetter, but we would need to handle ignoremissing
out of its __init__.
timeless - Oct. 26, 2016, 6:04 p.m.
FUJIWARA Katsunori wrote:
> +    If 'ignoremissing' is true, missing 'name' attribute doesn't cause
> +    abortion, and this function returns None. This is useful to

fwiw, this should be `abort`. `abortion` is only used in English with
regards to terminating pregnancies (occasionally it's misused to
describe preventing a pregnancy).

In context, an article ('an') should precede the noun (this applies
regardless of whether it's `abort` or `abortion`).

Note: I understand this is queued, these comments are mostly to help
people learn about how to use various English terms (in some cases to
avoid embarrassment). And also, at some point I hope to review my
emails and supply patches to address any that can be addressed (commit
messages that are public obviously can't be).

Patch

diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -182,6 +182,40 @@  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 subsequent 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 object to (1) assign a new value, and
+    (2) restore an original value to the attribute.
+
+    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))
+
+    origvalue = getattr(obj, name)
+    class attrutil(object):
+        def set(self, newvalue):
+            setattr(obj, name, newvalue)
+        def restore(self):
+            setattr(obj, name, origvalue)
+
+    return attrutil()
+
+# perf commands
+
 @command('perfwalk', formatteropts)
 def perfwalk(ui, repo, *pats, **opts):
     timer, fm = gettimer(ui, opts)