Page MenuHomePhabricator

changelog: use attrs instead of namedtuple
ClosedPublic

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

Details

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
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')

@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.)

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.