[PATCH] ztrsub() execution time / 2

[PATCH] ztrsub() execution time / 2

[Sebastian Gniazdowski]

[-- Attachment #1: Type: text/plain, Size: 893 bytes --]

Hello,
this optimization is a drop of callgrind instruction count by 43 mln
for ztrsub() – it had 89 mln instructions normally, now it has 46 mln:

ztrsub() callgrind I-count:
1> (89873326 - 46452836) / 1000000.0
43,4205

total callgrind I-count
2> (7906279976 - 7858975498) / 1000000.0
47,3045

 89,873,326  ztrsub [/usr/local/bin/zsh-5.6.2-dev-1-2noopt]
vs
 46,452,836  ztrsub [/usr/local/bin/zsh-5.6.2-dev-1-2yesopt]

Ran this callgrind probe twice to confirm. That said, `zsh -i -c exit'
nor a performance test script show any gain. I wonder why.. But
callgrind doesn't lie, and looking at the code one can understand why
I-count dropped by factor of 2. The patch has many context lines to
see whole function.
-- 
Sebastian Gniazdowski
News: https://twitter.com/ZdharmaI
IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin
Blog: http://zdharma.org

[-- Attachment #2: ztrsub_opt.diff.txt --]
[-- Type: text/plain, Size: 633 bytes --]

diff --git a/Src/utils.c b/Src/utils.c
index 914e30c..1eb8c71 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -5058,23 +5058,23 @@ ztrlenend(char const *s, char const *eptr)
 #endif /* MULTIBYTE_SUPPORT */
 
 /* Subtract two pointers in a metafied string. */
 
 /**/
 mod_export int
 ztrsub(char const *t, char const *s)
 {
     int l = t - s;
 
     while (s != t) {
-	if (*s++ == Meta) {
+	if (*s++ == Meta || (s != t && *s++ == Meta)) {
 #ifdef DEBUG
 	    if (! *s || s == t)
 		fprintf(stderr, "BUG: substring ends in the middle of a metachar in ztrsub()\n");
 	    else
 #endif
 	    s++;
 	    l--;
 	}
     }
     return l;
 }

Re: [PATCH] ztrsub() execution time / 2

[Daniel Shahaf]

Sebastian Gniazdowski wrote on Wed, Oct 17, 2018 at 06:29:08 +0200:
> diff --git a/Src/utils.c b/Src/utils.c
> index 914e30c..1eb8c71 100644
> --- a/Src/utils.c
> +++ b/Src/utils.c
> @@ -5058,23 +5058,23 @@ ztrlenend(char const *s, char const *eptr)
>  #endif /* MULTIBYTE_SUPPORT */
>  
>  /* Subtract two pointers in a metafied string. */
>  
>  /**/
>  mod_export int
>  ztrsub(char const *t, char const *s)
>  {
>      int l = t - s;
>  
>      while (s != t) {
> -	if (*s++ == Meta) {
> +	if (*s++ == Meta || (s != t && *s++ == Meta)) {

Thank you for generating the diff with additional context lines.

As far as I can tell, this patch is the sort of transformation that an
optimizing compiler should do by itself.  In fact, I'm having a hard time
seeing how the sequence of "BRANCH IF EQUAL" / "BRANCH IF NOT EQUAL" /
"INCREMENT" assembly instructions would be altered by the patch.

If your profiling was performed on zsh compiled with -O3, I would suggest that
you file a bug against your compiler's optimizer.

>  #ifdef DEBUG
>  	    if (! *s || s == t)
>  		fprintf(stderr, "BUG: substring ends in the middle of a metachar in ztrsub()\n");
>  	    else
>  #endif
>  	    s++;
>  	    l--;

Cheers,

Daniel

>  	}
>      }
>      return l;
>  }

Re: [PATCH] ztrsub() execution time / 2

[Sebastian Gniazdowski]

On Wed, 17 Oct 2018 at 16:25, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> As far as I can tell, this patch is the sort of transformation that an
> optimizing compiler should do by itself.  In fact, I'm having a hard time
> seeing how the sequence of "BRANCH IF EQUAL" / "BRANCH IF NOT EQUAL" /
> "INCREMENT" assembly instructions would be altered by the patch.
>
> If your profiling was performed on zsh compiled with -O3, I would suggest that
> you file a bug against your compiler's optimizer.

Why not "do it right, then do it yourself"? Why ignore -O0 zsh users?
Should I subscribe to compilers mailing lists and investigate this
well defined 1-line change, i.e. possible to grasp by logic?

--
Sebastian Gniazdowski
News: https://twitter.com/ZdharmaI
IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin
Blog: http://zdharma.org

Re: [PATCH] ztrsub() execution time / 2

[Daniel Shahaf]

Sebastian Gniazdowski wrote on Wed, 17 Oct 2018 21:02 +0200:
> On Wed, 17 Oct 2018 at 16:25, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > As far as I can tell, this patch is the sort of transformation that an
> > optimizing compiler should do by itself.  In fact, I'm having a hard time
> > seeing how the sequence of "BRANCH IF EQUAL" / "BRANCH IF NOT EQUAL" /
> > "INCREMENT" assembly instructions would be altered by the patch.
> >
> > If your profiling was performed on zsh compiled with -O3, I would suggest that
> > you file a bug against your compiler's optimizer.
> 
> Why not "do it right, then do it yourself"? Why ignore -O0 zsh users?
> Should I subscribe to compilers mailing lists and investigate this
> well defined 1-line change, i.e. possible to grasp by logic?

I have a hard time charitably interpreting what you just wrote.  It
doesn't come across as constructive.

I think the patch should not be applied because it makes the code harder
to read and to maintain.

Daniel