Patchwork D1938: [ui] Improve ui.write performance when not coloring on Windows

login
register
mail settings
Submitter phabricator
Date Jan. 25, 2018, 6:52 p.m.
Message ID <differential-rev-PHID-DREV-jecqblhga4xg33kjerhz-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/27059/
State Superseded
Headers show

Comments

phabricator - Jan. 25, 2018, 6:52 p.m.
joerg.sonnenberger created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  For ``hg diff``, ui.write appears as super hot in the profile with up to
  60% time spend in it for larger diffs. Introduce two predicates to
  decide if label processing is active at all and if it is, whether the
  result can still be simply batched up. If either case is true, process
  all chunks of a file in one go and use ``ui.write`` once. This reduces
  the time of ``hg diff`` from 3m54s to 2m26s for the test case.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1938

AFFECTED FILES
  mercurial/cmdutil.py
  mercurial/ui.py

CHANGE DETAILS




To: joerg.sonnenberger, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 25, 2018, 8:16 p.m.
joerg.sonnenberger added inline comments.

INLINE COMMENTS

> cmdutil.py:1627
> +        args = [iter(iterable)] * n
> +        return izip_longest(*args, fillvalue=('', ''))
> +

The grouping is necessary to avoid computing the full diff in memory with the associated memory use. Turns out splitting it up is even slightly faster, too. The grouper function itself is pretty generic (modulo the hard-coded fillvalue), so putting it in util.py or so would be a good idea.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1938

To: joerg.sonnenberger, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 26, 2018, 10:52 a.m.
lothiraldan added inline comments.

INLINE COMMENTS

> ui.py:887
> +        return self._colormode is None
> +    def canbatchlabelwrites(self, **opts):
> +        '''check if write calls with labels are batchable'''

Style nit, missing new line between the two methods

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1938

To: joerg.sonnenberger, #hg-reviewers
Cc: lothiraldan, mercurial-devel
phabricator - Feb. 3, 2018, 7:55 a.m.
yuja requested changes to this revision.
yuja added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> cmdutil.py:1593
>      else:
>          def write(s, **kw):
>              fp.write(s)

We no longer need this `write()` wrapper because no labeling is required if fp is given.

> cmdutil.py:1620
>      else:
> -        for chunk, label in patch.diffui(repo, node1, node2, match,
> -                                         changes, opts=diffopts, prefix=prefix,
> -                                         relroot=relroot,
> -                                         hunksfilterfn=hunksfilterfn):
> +        chunksiter = patch.diffui(repo, node1, node2, match, changes,
> +                                  opts=diffopts, prefix=prefix,

If we don't care labels, we can simply use `patch.diff()` and `.diffstat()` instead of `patch.diffui()` and `.diffstatui()` respectively.

> cmdutil.py:1624
> +
> +    from itertools import izip_longest
> +    def grouper(iterable, n):

Please move the import line to top.

> cmdutil.py:1629
> +
> +    if fp is None and ui.writenolabels():
> +        for block in grouper(chunksiter, 512):

`grouper()` could be replaced by `util.chunkbuffer()` over chunks of bytes.

  if nolabel:
      chunksiter = patch.diff()
      util.chunkbuffer(chunksiter)
  elif batchable:
      chunksiter = patch.diffui()
      util.chunkbuffer(ui.label(chunk, label) for chunk, label in chunksiter)

> ui.py:884
> +        if self._buffers and not opts.get(r'prompt', False):
> +            if not self._bufferapplylabels:
> +                return True

Perhaps we can simply make _colormode public instead of adding
these functions?

If ui is buffered, batchable state wouldn't matter as all write()s
will be combined into a string.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1938

To: joerg.sonnenberger, #hg-reviewers, yuja
Cc: yuja, lothiraldan, mercurial-devel

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -878,6 +878,18 @@ 
 
         return "".join(self._buffers.pop())
 
+    def writenolabels(self, **opts):
+        '''check if write actually uses the label'''
+        if self._buffers and not opts.get(r'prompt', False):
+            if not self._bufferapplylabels:
+                return True
+        return self._colormode is None
+    def canbatchlabelwrites(self, **opts):
+        '''check if write calls with labels are batchable'''
+        assert not self.writenolabels()
+        # Windows color printing is special, see ``write``.
+        return self._colormode != 'win32'
+
     def write(self, *args, **opts):
         '''write args to output
 
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -1615,14 +1615,21 @@ 
         chunks = patch.diff(repo, node1, node2, match, changes, opts=diffopts,
                             prefix=prefix, relroot=relroot,
                             hunksfilterfn=hunksfilterfn)
-        for chunk, label in patch.diffstatui(util.iterlines(chunks),
-                                             width=width):
-            write(chunk, label=label)
+        chunksiter = patch.diffstatui(util.iterlines(chunks), width=width)
     else:
-        for chunk, label in patch.diffui(repo, node1, node2, match,
-                                         changes, opts=diffopts, prefix=prefix,
-                                         relroot=relroot,
-                                         hunksfilterfn=hunksfilterfn):
+        chunksiter = patch.diffui(repo, node1, node2, match, changes,
+                                  opts=diffopts, prefix=prefix,
+                                  relroot=relroot, hunksfilterfn=hunksfilterfn)
+
+    if fp is None and ui.writenolabels():
+        write(*(chunk for chunk, label in chunksiter))
+    elif fp is None and ui.canbatchlabelwrites():
+        output = []
+        for chunk, label in chunksiter:
+            output.append(ui.label(chunk, label=label))
+        write(*output)
+    else:
+        for chunk, label in chunksiter:
             write(chunk, label=label)
 
     if listsubrepos: