This is an archive of the discontinued Mercurial Phabricator instance.

Differential D868

changelog: use attrs instead of namedtuple
ClosedPublic

Authored by sid0 on Oct 1 2017, 6:35 AM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGe51c8ffa1ffa: changelog: use attrs instead of namedtuple
Summary

See http://www.attrs.org/en/stable/why.html#namedtuples for why attrs are
better than namedtuples.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

sid0 created this revision.Oct 1 2017, 6:35 AM
durin42 accepted this revision.Oct 1 2017, 6:55 AM
This revision is now accepted and ready to land.Oct 1 2017, 6:55 AM
Closed by commit rHGe51c8ffa1ffa: changelog: use attrs instead of namedtuple (authored by sid0). · Explain WhyOct 1 2017, 7:34 AM
This revision was automatically updated to reflect the committed changes.
lothiraldan added a subscriber: lothiraldan.EditedOct 23 2017, 10:40 AM

We detected a small performance regression on this changeset thanks to our ASV instance: http://perf.octobus.net/#regressions?sort=3&dir=desc (look for e51c8ffa).

It seems to have slow down enough hg id -r . on Mercurial repository and hg diff on a clean Mercurial repository for ASV to detect it as a regression.

I'm dumping the raw results of asv compare because they are not available else-where if someone wants to take a look. Should I open a ticket on the bug tracker?

$ asv compare 765eb17a7eb8 e51c8ffa1ffa
Benchmarks that have stayed the same:

       before           after         ratio
     [765eb17a]       [e51c8ffa]
      82.2±0.01ms      83.8±0.01ms     1.02  basic_commands.LogTimeTestSuite.time_log_history('mercurial-2017', 10)
       218±0.01ms       221±0.06ms     1.01  basic_commands.LogTimeTestSuite.time_log_history('mercurial-2017', 100)
          1.25±0s          1.26±0s     1.00  basic_commands.LogTimeTestSuite.time_log_history('mercurial-2017', 1000)
          3.79±0s          3.79±0s     1.00  basic_commands.LogTimeTestSuite.time_log_history('mercurial-2017', 10000)
       150±0.02ms       152±0.01ms     1.01  basic_commands.LogTimeTestSuite.time_log_history('mozilla-central-2017', 10)
       165±0.02ms       166±0.01ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('mozilla-central-2017', 100)
        359±0.1ms        362±0.1ms     1.01  basic_commands.LogTimeTestSuite.time_log_history('mozilla-central-2017', 1000)
          2.52±0s          2.52±0s     1.00  basic_commands.LogTimeTestSuite.time_log_history('mozilla-central-2017', 10000)
       102±0.01ms          103±0ms     1.02  basic_commands.LogTimeTestSuite.time_log_history('netbeans-2017', 10)
       138±0.01ms       140±0.02ms     1.01  basic_commands.LogTimeTestSuite.time_log_history('netbeans-2017', 100)
        369±0.2ms       371±0.08ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('netbeans-2017', 1000)
         60.4±0ms         62.5±0ms     1.03  basic_commands.LogTimeTestSuite.time_log_history('pypy-2017', 10)
         78.1±0ms      80.1±0.01ms     1.03  basic_commands.LogTimeTestSuite.time_log_history('pypy-2017', 100)
       251±0.03ms       251±0.07ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('pypy-2017', 1000)
       7.57±0.01s          7.59±0s     1.00  basic_commands.LogTimeTestSuite.time_log_history('pypy-2017', 10000)
           67.2ms           69.1ms     1.03  basic_commands.TestSuite.track_commit('mercurial-2017')
            2.79s            2.80s     1.00  basic_commands.TestSuite.track_commit('mozilla-central-2017')
            1.54s            1.54s     1.00  basic_commands.TestSuite.track_commit('netbeans-2017')
            192ms            194ms     1.01  basic_commands.TestSuite.track_commit('pypy-2017')
         38.2±0ms         39.9±0ms     1.05  basic_commands.TimeTestSuite.time_bookmarks('mercurial-2017')
         44.7±0ms         46.8±0ms     1.05  basic_commands.TimeTestSuite.time_bookmarks('mozilla-central-2017')
         43.4±0ms         45.5±0ms     1.05  basic_commands.TimeTestSuite.time_bookmarks('netbeans-2017')
         38.5±0ms         40.6±0ms     1.05  basic_commands.TimeTestSuite.time_bookmarks('pypy-2017')
         46.7±0ms         48.3±0ms     1.04  basic_commands.TimeTestSuite.time_diff_tip('mercurial-2017')
       656±0.07ms        659±0.2ms     1.00  basic_commands.TimeTestSuite.time_diff_tip('mozilla-central-2017')
       342±0.03ms       344±0.02ms     1.01  basic_commands.TimeTestSuite.time_diff_tip('netbeans-2017')
         55.6±0ms         57.3±0ms     1.03  basic_commands.TimeTestSuite.time_diff_tip('pypy-2017')
         43.9±0ms       46.1±0.2ms     1.05  basic_commands.TimeTestSuite.time_emptydiff('mercurial-2017')
       870±0.08ms       873±0.03ms     1.00  basic_commands.TimeTestSuite.time_emptydiff('mozilla-central-2017')
       408±0.08ms       410±0.02ms     1.01  basic_commands.TimeTestSuite.time_emptydiff('netbeans-2017')
      57.1±0.01ms         58.9±0ms     1.03  basic_commands.TimeTestSuite.time_emptydiff('pypy-2017')
         48.7±0ms         50.4±0ms     1.04  basic_commands.TimeTestSuite.time_emptystatus('mercurial-2017')
          1.25±0s          1.25±0s     1.00  basic_commands.TimeTestSuite.time_emptystatus('mozilla-central-2017')
        762±0.9ms        763±0.3ms     1.00  basic_commands.TimeTestSuite.time_emptystatus('netbeans-2017')
      66.2±0.01ms         67.8±0ms     1.02  basic_commands.TimeTestSuite.time_emptystatus('pypy-2017')
         45.0±0ms         46.7±0ms     1.04  basic_commands.TimeTestSuite.time_id('mercurial-2017')
        872±0.1ms       874±0.01ms     1.00  basic_commands.TimeTestSuite.time_id('mozilla-central-2017')
        413±0.1ms       416±0.04ms     1.01  basic_commands.TimeTestSuite.time_id('netbeans-2017')
         57.7±0ms      59.5±0.01ms     1.03  basic_commands.TimeTestSuite.time_id('pypy-2017')
         43.3±0ms         44.9±0ms     1.04  basic_commands.TimeTestSuite.time_id_current('mercurial-2017')
       131±0.02ms       133±0.01ms     1.02  basic_commands.TimeTestSuite.time_id_current('mozilla-central-2017')
      95.6±0.01ms      97.2±0.01ms     1.02  basic_commands.TimeTestSuite.time_id_current('netbeans-2017')
         54.3±0ms      55.9±0.01ms     1.03  basic_commands.TimeTestSuite.time_id_current('pypy-2017')
         43.6±0ms         45.2±0ms     1.04  basic_commands.TimeTestSuite.time_log_tip('mercurial-2017')
         54.8±0ms         56.4±0ms     1.03  basic_commands.TimeTestSuite.time_log_tip('mozilla-central-2017')
         57.6±0ms         59.3±0ms     1.03  basic_commands.TimeTestSuite.time_log_tip('netbeans-2017')
         44.9±0ms         46.8±0ms     1.04  basic_commands.TimeTestSuite.time_log_tip('pypy-2017')
         45.2±0ms         46.8±0ms     1.04  basic_commands.TimeTestSuite.time_status_tip('mercurial-2017')
       579±0.07ms       581±0.09ms     1.00  basic_commands.TimeTestSuite.time_status_tip('mozilla-central-2017')
       341±0.02ms       343±0.04ms     1.00  basic_commands.TimeTestSuite.time_status_tip('netbeans-2017')
      53.7±0.01ms         55.4±0ms     1.03  basic_commands.TimeTestSuite.time_status_tip('pypy-2017')
         54.0±0ms         55.9±0ms     1.04  basic_commands.TimeTestSuite.time_summary('mercurial-2017')
          1.29±0s          1.29±0s     1.00  basic_commands.TimeTestSuite.time_summary('mozilla-central-2017')
        790±0.1ms       793±0.01ms     1.00  basic_commands.TimeTestSuite.time_summary('netbeans-2017')
         81.3±0ms         83.1±0ms     1.02  basic_commands.TimeTestSuite.time_summary('pypy-2017')
         31.2±0ms         31.3±0ms     1.00  basic_commands.TimeTestSuite.time_version('mercurial-2017')
         31.2±0ms         31.3±0ms     1.00  basic_commands.TimeTestSuite.time_version('mozilla-central-2017')
         31.2±0ms         31.3±0ms     1.00  basic_commands.TimeTestSuite.time_version('netbeans-2017')
         31.2±0ms         31.3±0ms     1.00  basic_commands.TimeTestSuite.time_version('pypy-2017')
durin42 added a comment.Oct 23 2017, 10:59 AM

@lothiraldan you very much buried the lede here (lots of data about things that didn't change, but no inline data on what didn't). The regressions appear to be:

basic_commands.TimeTestSuite.time_id_current('mercurial-2017') 43.922ms -> 46.190ms
basic_commands.TimeTestSuite.time_emptydiff('mercurial-2017') 43.185ms -> 45.353ms

My guess is that this has to do with the import cost of attr, and that it doesn't scale linearly with the size of the repo. I'm disinclined to be concerned about the 1-2ms for now, especially given that we'll be able to pay the attr import cost once, but namedtuples are known to be expensive *per namedtuple* (so as we move from namedtuple to attr we should make this loss back.)

(Other reviewers: please chime in if you disagree. I know there's a slippery slope for perf here, but I'm not inclined to worry at all about 44 vs 46 ms for id, or 43 vs 45 for empty-diff. It very much looks like once any actual statting has to be done the import cost is lost in the noise.)

lothiraldan added a comment.Oct 23 2017, 1:33 PM

The perf difference is not big, but I'm not sure if it's due to the import time of attrs because basic_commands.TimeTestSuite.time_version didn't change much. I have the impression that we pay a cost the first time an attrs-based class is instantiated.

I've pasted all the data because they are a lot of tests (but not all) that became worst with this changeset (2-3 ms on average I would say) so if someone also finds it fishy, maybe we should take a look?

For comparison, here is asv compare between this changeset grand-parent and its parent:

asv compare 9fb9f8440b71 765eb17a7eb8 -s

Benchmarks that have stayed the same:

       before           after         ratio
     [9fb9f844]       [765eb17a]
      82.3±0.02ms      82.2±0.01ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('mercurial-2017', 10)
       218±0.07ms       218±0.01ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('mercurial-2017', 100)
          1.26±0s          1.25±0s     1.00  basic_commands.LogTimeTestSuite.time_log_history('mercurial-2017', 1000)
          3.81±0s          3.79±0s     0.99  basic_commands.LogTimeTestSuite.time_log_history('mercurial-2017', 10000)
       150±0.02ms       150±0.02ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('mozilla-central-2017', 10)
       165±0.02ms       165±0.02ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('mozilla-central-2017', 100)
       359±0.09ms        359±0.1ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('mozilla-central-2017', 1000)
          2.52±0s          2.52±0s     1.00  basic_commands.LogTimeTestSuite.time_log_history('mozilla-central-2017', 10000)
          102±0ms       102±0.01ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('netbeans-2017', 10)
       139±0.01ms       138±0.01ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('netbeans-2017', 100)
        370±0.2ms        369±0.2ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('netbeans-2017', 1000)
         60.4±0ms         60.4±0ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('pypy-2017', 10)
         78.2±0ms         78.1±0ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('pypy-2017', 100)
       251±0.05ms       251±0.03ms     1.00  basic_commands.LogTimeTestSuite.time_log_history('pypy-2017', 1000)
          7.62±0s       7.57±0.01s     0.99  basic_commands.LogTimeTestSuite.time_log_history('pypy-2017', 10000)
           67.1ms           67.2ms     1.00  basic_commands.TestSuite.track_commit('mercurial-2017')
            2.79s            2.79s     1.00  basic_commands.TestSuite.track_commit('mozilla-central-2017')
            1.54s            1.54s     1.00  basic_commands.TestSuite.track_commit('netbeans-2017')
            192ms            192ms     1.00  basic_commands.TestSuite.track_commit('pypy-2017')
         38.3±0ms         38.2±0ms     1.00  basic_commands.TimeTestSuite.time_bookmarks('mercurial-2017')
         44.8±0ms         44.7±0ms     1.00  basic_commands.TimeTestSuite.time_bookmarks('mozilla-central-2017')
         43.5±0ms         43.4±0ms     1.00  basic_commands.TimeTestSuite.time_bookmarks('netbeans-2017')
         38.7±0ms         38.5±0ms     1.00  basic_commands.TimeTestSuite.time_bookmarks('pypy-2017')
         46.7±0ms         46.7±0ms     1.00  basic_commands.TimeTestSuite.time_diff_tip('mercurial-2017')
        657±0.2ms       656±0.07ms     1.00  basic_commands.TimeTestSuite.time_diff_tip('mozilla-central-2017')
       342±0.01ms       342±0.03ms     1.00  basic_commands.TimeTestSuite.time_diff_tip('netbeans-2017')
         55.7±0ms         55.6±0ms     1.00  basic_commands.TimeTestSuite.time_diff_tip('pypy-2017')
         44.1±0ms         43.9±0ms     1.00  basic_commands.TimeTestSuite.time_emptydiff('mercurial-2017')
        869±0.2ms       870±0.08ms     1.00  basic_commands.TimeTestSuite.time_emptydiff('mozilla-central-2017')
       408±0.05ms       408±0.08ms     1.00  basic_commands.TimeTestSuite.time_emptydiff('netbeans-2017')
         57.2±0ms      57.1±0.01ms     1.00  basic_commands.TimeTestSuite.time_emptydiff('pypy-2017')
         48.8±0ms         48.7±0ms     1.00  basic_commands.TimeTestSuite.time_emptystatus('mercurial-2017')
          1.25±0s          1.25±0s     1.00  basic_commands.TimeTestSuite.time_emptystatus('mozilla-central-2017')
        763±0.1ms        762±0.9ms     1.00  basic_commands.TimeTestSuite.time_emptystatus('netbeans-2017')
         66.3±0ms      66.2±0.01ms     1.00  basic_commands.TimeTestSuite.time_emptystatus('pypy-2017')
         45.1±0ms         45.0±0ms     1.00  basic_commands.TimeTestSuite.time_id('mercurial-2017')
        871±0.1ms        872±0.1ms     1.00  basic_commands.TimeTestSuite.time_id('mozilla-central-2017')
       412±0.05ms        413±0.1ms     1.00  basic_commands.TimeTestSuite.time_id('netbeans-2017')
         57.9±0ms         57.7±0ms     1.00  basic_commands.TimeTestSuite.time_id('pypy-2017')
         43.3±0ms         43.3±0ms     1.00  basic_commands.TimeTestSuite.time_id_current('mercurial-2017')
       131±0.03ms       131±0.02ms     1.00  basic_commands.TimeTestSuite.time_id_current('mozilla-central-2017')
      95.7±0.01ms      95.6±0.01ms     1.00  basic_commands.TimeTestSuite.time_id_current('netbeans-2017')
      54.4±0.01ms         54.3±0ms     1.00  basic_commands.TimeTestSuite.time_id_current('pypy-2017')
         43.6±0ms         43.6±0ms     1.00  basic_commands.TimeTestSuite.time_log_tip('mercurial-2017')
         54.8±0ms         54.8±0ms     1.00  basic_commands.TimeTestSuite.time_log_tip('mozilla-central-2017')
         57.7±0ms         57.6±0ms     1.00  basic_commands.TimeTestSuite.time_log_tip('netbeans-2017')
         44.9±0ms         44.9±0ms     1.00  basic_commands.TimeTestSuite.time_log_tip('pypy-2017')
         45.2±0ms         45.2±0ms     1.00  basic_commands.TimeTestSuite.time_status_tip('mercurial-2017')
          580±0ms       579±0.07ms     1.00  basic_commands.TimeTestSuite.time_status_tip('mozilla-central-2017')
       341±0.01ms       341±0.02ms     1.00  basic_commands.TimeTestSuite.time_status_tip('netbeans-2017')
         53.7±0ms      53.7±0.01ms     1.00  basic_commands.TimeTestSuite.time_status_tip('pypy-2017')
         54.1±0ms         54.0±0ms     1.00  basic_commands.TimeTestSuite.time_summary('mercurial-2017')
          1.29±0s          1.29±0s     1.00  basic_commands.TimeTestSuite.time_summary('mozilla-central-2017')
        788±0.1ms        790±0.1ms     1.00  basic_commands.TimeTestSuite.time_summary('netbeans-2017')
         81.4±0ms         81.3±0ms     1.00  basic_commands.TimeTestSuite.time_summary('pypy-2017')
         31.3±0ms         31.2±0ms     1.00  basic_commands.TimeTestSuite.time_version('mercurial-2017')
         31.3±0ms         31.2±0ms     1.00  basic_commands.TimeTestSuite.time_version('mozilla-central-2017')
         31.3±0ms         31.2±0ms     1.00  basic_commands.TimeTestSuite.time_version('netbeans-2017')
         31.3±0ms         31.2±0ms     1.00  basic_commands.TimeTestSuite.time_version('pypy-2017')

As you can see we are pretty confident that's it's not just jitters or instability.

Revision Contents
Changeset List

PathPackages
Mmercurial/changelog.py (28 lines)

Diff 2252

mercurial/changelog.py

# changelog.py - changelog class for mercurial # changelog.py - changelog class for mercurial
# #
# Copyright 2005-2007 Matt Mackall <mpm@selenic.com> # Copyright 2005-2007 Matt Mackall <mpm@selenic.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import collections
from .i18n import _ from .i18n import _
from .node import ( from .node import (
bin, bin,
hex, hex,
nullid, nullid,
) )
from .thirdparty import (
attr,
)
from . import ( from . import (
encoding, encoding,
error, error,
revlog, revlog,
util, util,
) )
def _delayopener(opener, target, buf): def _delayopener(opener, target, buf):
"""build an opener that stores chunks in 'buf' instead of 'target'""" """build an opener that stores chunks in 'buf' instead of 'target'"""
def _delay(name, mode='r', checkambig=False): def _delay(name, mode='r', checkambig=False):
if name != target: if name != target:
return opener(name, mode) return opener(name, mode)
return appender(opener, name, mode, buf) return appender(opener, name, mode, buf)
return _delay return _delay
_changelogrevision = collections.namedtuple(u'changelogrevision', @attr.s
(u'manifest', u'user', u'date', class _changelogrevision(object):
u'files', u'description', # Extensions might modify _defaultextra, so let the constructor below pass
u'extra')) # it in
extra = attr.ib()
manifest = attr.ib(default=nullid)
user = attr.ib(default='')
date = attr.ib(default=(0, 0))
files = attr.ib(default=[])
description = attr.ib(default='')
class changelogrevision(object): class changelogrevision(object):
"""Holds results of a parsed changelog revision. """Holds results of a parsed changelog revision.
Changelog revisions consist of multiple pieces of data, including Changelog revisions consist of multiple pieces of data, including
the manifest node, user, and date. This object exposes a view into the manifest node, user, and date. This object exposes a view into
the parsed object. the parsed object.
""" """
__slots__ = ( __slots__ = (
u'_offsets', u'_offsets',
u'_text', u'_text',
) )
def __new__(cls, text): def __new__(cls, text):
if not text: if not text:
return _changelogrevision( return _changelogrevision(extra=_defaultextra)
manifest=nullid,
user='',
date=(0, 0),
files=[],
description='',
extra=_defaultextra,
)
self = super(changelogrevision, cls).__new__(cls) self = super(changelogrevision, cls).__new__(cls)
# We could return here and implement the following as an __init__. # We could return here and implement the following as an __init__.
# But doing it here is equivalent and saves an extra function call. # But doing it here is equivalent and saves an extra function call.
# format used: # format used:
# nodeid\n : manifest node in ascii # nodeid\n : manifest node in ascii
# user\n : user, no \n or \r allowed # user\n : user, no \n or \r allowed