Patchwork [5,of,5] ui: avoid needless casting to a str

login
register
mail settings
Submitter Gregory Szorc
Date Nov. 24, 2015, 9:11 p.m.
Message ID <5d5af88a927bbffa921f.1448399502@gps-mbp.local>
Download mbox | patch
Permalink /patch/11609/
State Accepted
Headers show

Comments

Gregory Szorc - Nov. 24, 2015, 9:11 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1448232295 28800
#      Sun Nov 22 14:44:55 2015 -0800
# Node ID 5d5af88a927bbffa921f76f6600e0e67475df369
# Parent  e3c27c5cece623b52b053f84698446855e908396
ui: avoid needless casting to a str

In many cases, we don't need to cast to a str because the object will
be cast when it is eventually written.

As part of testing this, I added some code to raise exceptions when a
non-str was passed in and wasn't able to trigger it. i.e. we're already
passing str into this function everywhere, so the casting isn't
necessary.
Augie Fackler - Nov. 24, 2015, 11:57 p.m.
On Tue, Nov 24, 2015 at 01:11:42PM -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1448232295 28800
> #      Sun Nov 22 14:44:55 2015 -0800
> # Node ID 5d5af88a927bbffa921f76f6600e0e67475df369
> # Parent  e3c27c5cece623b52b053f84698446855e908396
> ui: avoid needless casting to a str

Queued these too, they seem simple enough.

>
> In many cases, we don't need to cast to a str because the object will
> be cast when it is eventually written.
>
> As part of testing this, I added some code to raise exceptions when a
> non-str was passed in and wasn't able to trigger it. i.e. we're already
> passing str into this function everywhere, so the casting isn't
> necessary.
>
> diff --git a/hgext/color.py b/hgext/color.py
> --- a/hgext/color.py
> +++ b/hgext/color.py
> @@ -433,18 +433,17 @@ class colorui(uimod.ui):
>
>          label = opts.get('label', '')
>          if self._buffers:
>              if self._bufferapplylabels:
> -                self._buffers[-1].extend(self.label(str(a), label)
> -                                         for a in args)
> +                self._buffers[-1].extend(self.label(a, label) for a in args)
>              else:
> -                self._buffers[-1].extend(str(a) for a in args)
> +                self._buffers[-1].extend(args)
>          elif self._colormode == 'win32':
>              for a in args:
>                  win32print(a, super(colorui, self).write, **opts)
>          else:
>              return super(colorui, self).write(
> -                *[self.label(str(a), label) for a in args], **opts)
> +                *[self.label(a, label) for a in args], **opts)
>
>      def write_err(self, *args, **opts):
>          if self._colormode is None:
>              return super(colorui, self).write_err(*args, **opts)
> @@ -456,9 +455,9 @@ class colorui(uimod.ui):
>              for a in args:
>                  win32print(a, super(colorui, self).write_err, **opts)
>          else:
>              return super(colorui, self).write_err(
> -                *[self.label(str(a), label) for a in args], **opts)
> +                *[self.label(a, label) for a in args], **opts)
>
>      def showlabel(self, msg, label):
>          if label and msg:
>              if msg[-1] == '\n':
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -622,12 +622,12 @@ class ui(object):
>          a label of "status.modified" for modified files.
>          '''
>          self._progclear()
>          if self._buffers:
> -            self._buffers[-1].extend([str(a) for a in args])
> +            self._buffers[-1].extend(a for a in args)
>          else:
>              for a in args:
> -                self.fout.write(str(a))
> +                self.fout.write(a)
>
>      def write_err(self, *args, **opts):
>          self._progclear()
>          try:
> @@ -635,9 +635,9 @@ class ui(object):
>                  return self.write(*args, **opts)
>              if not getattr(self.fout, 'closed', False):
>                  self.fout.flush()
>              for a in args:
> -                self.ferr.write(str(a))
> +                self.ferr.write(a)
>              # stderr may be buffered under win32 when redirected to files,
>              # including stdout.
>              if not getattr(self.ferr, 'closed', False):
>                  self.ferr.flush()
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Yuya Nishihara - Nov. 25, 2015, 12:55 p.m.
On Tue, 24 Nov 2015 13:11:42 -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1448232295 28800
> #      Sun Nov 22 14:44:55 2015 -0800
> # Node ID 5d5af88a927bbffa921f76f6600e0e67475df369
> # Parent  e3c27c5cece623b52b053f84698446855e908396
> ui: avoid needless casting to a str
> 
> In many cases, we don't need to cast to a str because the object will
> be cast when it is eventually written.
> 
> As part of testing this, I added some code to raise exceptions when a
> non-str was passed in and wasn't able to trigger it. i.e. we're already
> passing str into this function everywhere, so the casting isn't
> necessary.
> 
> diff --git a/hgext/color.py b/hgext/color.py
> --- a/hgext/color.py
> +++ b/hgext/color.py
> @@ -433,18 +433,17 @@ class colorui(uimod.ui):
>  
>          label = opts.get('label', '')
>          if self._buffers:
>              if self._bufferapplylabels:
> -                self._buffers[-1].extend(self.label(str(a), label)
> -                                         for a in args)
> +                self._buffers[-1].extend(self.label(a, label) for a in args)
>              else:
> -                self._buffers[-1].extend(str(a) for a in args)
> +                self._buffers[-1].extend(args)
>          elif self._colormode == 'win32':
>              for a in args:
>                  win32print(a, super(colorui, self).write, **opts)
>          else:
>              return super(colorui, self).write(
> -                *[self.label(str(a), label) for a in args], **opts)
> +                *[self.label(a, label) for a in args], **opts)
>  
>      def write_err(self, *args, **opts):
>          if self._colormode is None:
>              return super(colorui, self).write_err(*args, **opts)
> @@ -456,9 +455,9 @@ class colorui(uimod.ui):
>              for a in args:
>                  win32print(a, super(colorui, self).write_err, **opts)
>          else:
>              return super(colorui, self).write_err(
> -                *[self.label(str(a), label) for a in args], **opts)
> +                *[self.label(a, label) for a in args], **opts)
>  
>      def showlabel(self, msg, label):
>          if label and msg:
>              if msg[-1] == '\n':
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -622,12 +622,12 @@ class ui(object):
>          a label of "status.modified" for modified files.
>          '''
>          self._progclear()
>          if self._buffers:
> -            self._buffers[-1].extend([str(a) for a in args])
> +            self._buffers[-1].extend(a for a in args)

This is somewhat related. Can we call self.label(a, label) here so that
colorui won't have to care about _bufferapplylabels?

Or we can't because it would make things slow?
Gregory Szorc - Dec. 4, 2015, 6:01 a.m.
On Wed, Nov 25, 2015 at 4:55 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Tue, 24 Nov 2015 13:11:42 -0800, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1448232295 28800
> > #      Sun Nov 22 14:44:55 2015 -0800
> > # Node ID 5d5af88a927bbffa921f76f6600e0e67475df369
> > # Parent  e3c27c5cece623b52b053f84698446855e908396
> > ui: avoid needless casting to a str
> >
> > In many cases, we don't need to cast to a str because the object will
> > be cast when it is eventually written.
> >
> > As part of testing this, I added some code to raise exceptions when a
> > non-str was passed in and wasn't able to trigger it. i.e. we're already
> > passing str into this function everywhere, so the casting isn't
> > necessary.
> >
> > diff --git a/hgext/color.py b/hgext/color.py
> > --- a/hgext/color.py
> > +++ b/hgext/color.py
> > @@ -433,18 +433,17 @@ class colorui(uimod.ui):
> >
> >          label = opts.get('label', '')
> >          if self._buffers:
> >              if self._bufferapplylabels:
> > -                self._buffers[-1].extend(self.label(str(a), label)
> > -                                         for a in args)
> > +                self._buffers[-1].extend(self.label(a, label) for a in
> args)
> >              else:
> > -                self._buffers[-1].extend(str(a) for a in args)
> > +                self._buffers[-1].extend(args)
> >          elif self._colormode == 'win32':
> >              for a in args:
> >                  win32print(a, super(colorui, self).write, **opts)
> >          else:
> >              return super(colorui, self).write(
> > -                *[self.label(str(a), label) for a in args], **opts)
> > +                *[self.label(a, label) for a in args], **opts)
> >
> >      def write_err(self, *args, **opts):
> >          if self._colormode is None:
> >              return super(colorui, self).write_err(*args, **opts)
> > @@ -456,9 +455,9 @@ class colorui(uimod.ui):
> >              for a in args:
> >                  win32print(a, super(colorui, self).write_err, **opts)
> >          else:
> >              return super(colorui, self).write_err(
> > -                *[self.label(str(a), label) for a in args], **opts)
> > +                *[self.label(a, label) for a in args], **opts)
> >
> >      def showlabel(self, msg, label):
> >          if label and msg:
> >              if msg[-1] == '\n':
> > diff --git a/mercurial/ui.py b/mercurial/ui.py
> > --- a/mercurial/ui.py
> > +++ b/mercurial/ui.py
> > @@ -622,12 +622,12 @@ class ui(object):
> >          a label of "status.modified" for modified files.
> >          '''
> >          self._progclear()
> >          if self._buffers:
> > -            self._buffers[-1].extend([str(a) for a in args])
> > +            self._buffers[-1].extend(a for a in args)
>
> This is somewhat related. Can we call self.label(a, label) here so that
> colorui won't have to care about _bufferapplylabels?
>
> Or we can't because it would make things slow?
>

When I spent a few hours trying to figure out how to make `hg log` as fast
as possible, color definitely added noticeable overhead over plain. Even
some % string formatting shows up in statprof for `hg log`!

I didn't measure this specifically, but I suspect with the high frequency
write() is called that the function call overhead would have an impact.

Patch

diff --git a/hgext/color.py b/hgext/color.py
--- a/hgext/color.py
+++ b/hgext/color.py
@@ -433,18 +433,17 @@  class colorui(uimod.ui):
 
         label = opts.get('label', '')
         if self._buffers:
             if self._bufferapplylabels:
-                self._buffers[-1].extend(self.label(str(a), label)
-                                         for a in args)
+                self._buffers[-1].extend(self.label(a, label) for a in args)
             else:
-                self._buffers[-1].extend(str(a) for a in args)
+                self._buffers[-1].extend(args)
         elif self._colormode == 'win32':
             for a in args:
                 win32print(a, super(colorui, self).write, **opts)
         else:
             return super(colorui, self).write(
-                *[self.label(str(a), label) for a in args], **opts)
+                *[self.label(a, label) for a in args], **opts)
 
     def write_err(self, *args, **opts):
         if self._colormode is None:
             return super(colorui, self).write_err(*args, **opts)
@@ -456,9 +455,9 @@  class colorui(uimod.ui):
             for a in args:
                 win32print(a, super(colorui, self).write_err, **opts)
         else:
             return super(colorui, self).write_err(
-                *[self.label(str(a), label) for a in args], **opts)
+                *[self.label(a, label) for a in args], **opts)
 
     def showlabel(self, msg, label):
         if label and msg:
             if msg[-1] == '\n':
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -622,12 +622,12 @@  class ui(object):
         a label of "status.modified" for modified files.
         '''
         self._progclear()
         if self._buffers:
-            self._buffers[-1].extend([str(a) for a in args])
+            self._buffers[-1].extend(a for a in args)
         else:
             for a in args:
-                self.fout.write(str(a))
+                self.fout.write(a)
 
     def write_err(self, *args, **opts):
         self._progclear()
         try:
@@ -635,9 +635,9 @@  class ui(object):
                 return self.write(*args, **opts)
             if not getattr(self.fout, 'closed', False):
                 self.fout.flush()
             for a in args:
-                self.ferr.write(str(a))
+                self.ferr.write(a)
             # stderr may be buffered under win32 when redirected to files,
             # including stdout.
             if not getattr(self.ferr, 'closed', False):
                 self.ferr.flush()