Use strings.TrimRightFunc instead of strings.TrimRight to omit internal rune comparison function allocation

[codecov[bot]]

Codecov Report

Merging #29 (516e0a8) into master (0b2f4c4) will increase coverage by 0.13%. The diff coverage is 100.00%.

:exclamation: Current head 516e0a8 differs from pull request most recent head 6f1dda1. Consider uploading reports for the commit 6f1dda1 to get more accurate results

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
+ Coverage   91.25%   91.38%   +0.13%     
==========================================
  Files           3        3              
  Lines         263      267       +4     
==========================================
+ Hits          240      244       +4     
  Misses         12       12              
  Partials       11       11              

Impacted Files	Coverage Δ
parser.go	90.90% <100.00%> (+0.31%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0b2f4c4...6f1dda1. Read the comment docs.

[moredure]

Yeap, sure, @temoto, is it possible that more than one asterisk might happen in the end of line?

[moredure]

func BenchmarkTrimSuffix(b *testing.B) {
    const line = "Allow: *"
    b.Run("HasSuffix+TrimRight", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            if strings.HasSuffix(line, "*") {
                _ = strings.TrimRight(line, "*")
            }
        }
    })
    b.Run("HasSuffix+TrimRightFunc", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            if strings.HasSuffix(line, "*") {
                _ = strings.TrimRightFunc(line, func (r rune) bool {
                    return r == '*'
                })
            }
        }
    })
    b.Run("TrimRightFunc", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            _ = strings.TrimRightFunc(line, func (r rune) bool {
                return r == '*'
            })
        }
    })
    b.Run("TrimSuffix", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            _ = strings.TrimSuffix(line, "*")
        }
    })

    b.Run("TrimSuffixManually", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            if strings.HasSuffix(line, "*") {
                _ = line[:len(line)-1]
            }
        }
    })
}

cpu: Intel(R) Core(TM) i5-7600 CPU @ 3.50GHz
BenchmarkTrimSuffix
BenchmarkTrimSuffix/HasSuffix+TrimRight
BenchmarkTrimSuffix/HasSuffix+TrimRight-4           24147571            45.49 ns/op       24 B/op          1 allocs/op
BenchmarkTrimSuffix/HasSuffix+TrimRightFunc
BenchmarkTrimSuffix/HasSuffix+TrimRightFunc-4       100000000           11.85 ns/op        0 B/op          0 allocs/op
BenchmarkTrimSuffix/TrimRightFunc
BenchmarkTrimSuffix/TrimRightFunc-4                 100000000           11.19 ns/op        0 B/op          0 allocs/op
BenchmarkTrimSuffix/TrimSuffix
BenchmarkTrimSuffix/TrimSuffix-4                    1000000000           0.2609 ns/op          0 B/op          0 allocs/op
BenchmarkTrimSuffix/TrimSuffixManually
BenchmarkTrimSuffix/TrimSuffixManually-4            1000000000           0.2520 ns/op          0 B/op          0 allocs/op

[temoto]

No, I mean script/bench which runs full parsing benchmarks against master and modified code.

Generally one should expect robots.txt content to be anything. Two or more asterisks is definitely possible in the wild web.

[temoto]

Also, according to this benchmark output, TrimSuffix is as good as slice, maybe because compiler removed it because of _ = foo.

[temoto]

11ns sounds pretty good to me. Maybe it should be just TrimRightFunc as in your first patch.

I'd think unexported module level function produce best result, but compilers will always surprise.

package robotstxt
func isAsterisk(r rune) bool { return r == '*' }

And thanks for making this library faster. Much appreciated.

[moredure]

Getting rid of HasSuffix becomes much faster

func BenchmarkTrimSuffix(b *testing.B) {
    var line = "Allow: *"
    var x string
    b.Run("HasSuffix+TrimRight", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            if strings.HasSuffix(line, "*") {
                x = strings.TrimRight(line, "*")
            }
        }
    })
    b.Run("HasSuffix+TrimRightFunc", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            if strings.HasSuffix(line, "*") {
                x = strings.TrimRightFunc(line, func (r rune) bool {
                    return r == '*'
                })
            }
        }
    })
    b.Run("TrimRightFunc", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            x = strings.TrimRightFunc(line, func (r rune) bool {
                return r == '*'
            })
        }
    })
    b.Run("TrimSuffix", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            x = strings.TrimSuffix(line, "*")
        }
    })

    b.Run("TrimSuffixManually", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            if z := len(line); z > 0 && line[z-1:] == "*" {
                x = line[0:z-1]
            }
        }
    })
    b.Run("TrimSuffixManuallyCanHandleMultipleAsterisk", func(b *testing.B) {
        b.ReportAllocs()
        for i := 0; i < b.N; i++ {
            x = line
            for z := len(x); z > 0 && x[z-1] == '*'; {
                z -= 1
                x = x[:z]
            }
        }
    })
    runtime.KeepAlive(x)
}

cpu: Intel(R) Core(TM) i5-7600 CPU @ 3.50GHz
BenchmarkTrimSuffix
BenchmarkTrimSuffix/HasSuffix+TrimRight
BenchmarkTrimSuffix/HasSuffix+TrimRight-4           23404965            47.33 ns/op       24 B/op          1 allocs/op
BenchmarkTrimSuffix/HasSuffix+TrimRightFunc
BenchmarkTrimSuffix/HasSuffix+TrimRightFunc-4       79569001            14.73 ns/op        0 B/op          0 allocs/op
BenchmarkTrimSuffix/TrimRightFunc
BenchmarkTrimSuffix/TrimRightFunc-4                 93475473            12.43 ns/op        0 B/op          0 allocs/op
BenchmarkTrimSuffix/TrimSuffix
BenchmarkTrimSuffix/TrimSuffix-4                    317824923            3.762 ns/op           0 B/op          0 allocs/op
BenchmarkTrimSuffix/TrimSuffixManually
BenchmarkTrimSuffix/TrimSuffixManually-4            1000000000           0.8567 ns/op          0 B/op          0 allocs/op
BenchmarkTrimSuffix/TrimSuffixManuallyCanHandleMultipleAsterisk
BenchmarkTrimSuffix/TrimSuffixManuallyCanHandleMultipleAsterisk-4           648310034            1.843 ns/op           0 B/op          0 allocs/op

[moredure]

The fastest one is

for z := len(x); z > 0; {
  z--
  if x[z] != '*' {
      break
  }
  x = x[:z]
}

but it's only 0.009ns faster )))

[temoto]

I think it is too complicated to understand.

On Wed, Mar 24, 2021, 16:39 Mikhail Faraponov @.***> wrote:

The fastest one is

for z := len(x); z > 0; { z-- if x[z] != '*' { break } x = x[:z] }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/temoto/robotstxt/pull/29#issuecomment-805829918, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGTMI57JIO4L5Z424352LTFHTSTANCNFSM4ZVEEEPA .

[moredure]

The funny part, when I separated this code into separete code block it becomes faster: go 1.16.2, maybe inlining not working so good in older versions

func TrimAsterisk(x string) string {
    for z := len(x); z > 0; {
        z--
        if x[z] != '*' {
            return x
        }
        x = x[:z]
    }
    return x
}

[moredure]

But this way it will not handle possible utf-8 runes (two bytes symbols, which may contain dot ascii code)

[temoto]

So in current project benchmark suite there is not enough disallow: * lines, I've run TrimRightFunc and custom TrimRightAsterisk against master.

master vs TrimRightFunc

benchmark                         old ns/op     new ns/op     delta
BenchmarkParseFromString001-4     2855          2710          -5.08%
BenchmarkParseFromStatus401-4     24.5          23.7          -3.27%
BenchmarkParseFromString002-4     304489        307000        +0.82%

benchmark                         old allocs     new allocs     delta
BenchmarkParseFromString001-4     14             13             -7.14%
BenchmarkParseFromStatus401-4     0              0              +0.00%
BenchmarkParseFromString002-4     1485           1485           +0.00%

master vs TrimRightAsterisk

benchmark                         old ns/op     new ns/op     delta
BenchmarkParseFromString001-4     2855          2742          -3.96%
BenchmarkParseFromStatus401-4     24.5          24.0          -2.04%
BenchmarkParseFromString002-4     304489        302218        -0.75%

benchmark                         old allocs     new allocs     delta
BenchmarkParseFromString001-4     14             13             -7.14%
BenchmarkParseFromStatus401-4     0              0              +0.00%
BenchmarkParseFromString002-4     1485           1485           +0.00%

TrimRightFunc is both more readable and faster. I agree needless allocation was a problem.

[temoto]

Thank you very much.