Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

image: remove bounds check in "At" and "Set" methods, with re slice #14884

Closed
pierrre opened this issue Mar 20, 2016 · 7 comments
Closed

image: remove bounds check in "At" and "Set" methods, with re slice #14884

pierrre opened this issue Mar 20, 2016 · 7 comments

Comments

@pierrre
Copy link

@pierrre pierrre commented Mar 20, 2016

  1. What version of Go are you using (go version)?
    1.6
  2. What operating system and processor architecture are you using (go env)?
    linux amd64

I just saw this https://go-review.googlesource.com/#/c/20654/

Could we rewrite

func (p *RGBA64) RGBA64At(x, y int) color.RGBA64 {
    if !(Point{x, y}.In(p.Rect)) {
        return color.RGBA64{}
    }
    i := p.PixOffset(x, y)
    return color.RGBA64{
        uint16(p.Pix[i+0])<<8 | uint16(p.Pix[i+1]),
        uint16(p.Pix[i+2])<<8 | uint16(p.Pix[i+3]),
        uint16(p.Pix[i+4])<<8 | uint16(p.Pix[i+5]),
        uint16(p.Pix[i+6])<<8 | uint16(p.Pix[i+7]),
    }
}

to

func (p *RGBA64) RGBA64At(x, y int) color.RGBA64 {
    if !(Point{x, y}.In(p.Rect)) {
        return color.RGBA64{}
    }
    i := p.PixOffset(x, y)
    s := p.Pix[i:i+8]
    return color.RGBA64{
        uint16(s[0])<<8 | uint16(s[1]),
        uint16(s[2])<<8 | uint16(s[3]),
        uint16(s[4])<<8 | uint16(s[5]),
        uint16(s[6])<<8 | uint16(s[7]),
    }
}

I did some benchmarks on my package https://github.com/pierrre/imageserver/tree/master/image/internal (that borrows code from image).
And I see interesting results.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Mar 20, 2016
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 20, 2016

Loading

@nigeltao
Copy link
Contributor

@nigeltao nigeltao commented Mar 21, 2016

Yeah, I'd be willing to accept a CL, if the results are sufficiently interesting.

Loading

@pierrre
Copy link
Author

@pierrre pierrre commented Mar 21, 2016

How to check if bounds check are really removed?

Should I read assembly code with go build -gcflags="-S"?
What do they look like?

Loading

@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 21, 2016

On Mon, Mar 21, 2016 at 1:15 AM, Pierre Durand notifications@github.com
wrote:

How to check if bounds check are really removed?

Should I read assembly code with go build -gcflags="-S"?
What do they look like?

Yes, that will work. They are just a compare and branch where the
destination is a call to panicindex (or panicslice, for slice bounds
checks).

We could use a -d=bounds to print all the places where a bounds check
happens. We have -d=nil and -d=wb for nil checks and write barriers,
respectively. It might be kind of tricky to implement though.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#14884 (comment)

Loading

@brtzsnr
Copy link
Contributor

@brtzsnr brtzsnr commented Mar 21, 2016

FWIW, the correct line to use is s := p.Pix[i:i+8:len(p.Pix)].

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 22, 2018

Change https://golang.org/cl/136796 mentions this issue: image: add benchmarks for At and Set methods

Loading

gopherbot pushed a commit that referenced this issue Sep 24, 2018
Added in preparation for looking at some optimizations around bounds
checks.

BenchmarkAt/rgba-8            100000000     18.5 ns/op      4 B/op   1 allocs/op
BenchmarkAt/rgba64-8          100000000     22.9 ns/op      8 B/op   1 allocs/op
BenchmarkAt/nrgba-8           100000000     18.8 ns/op      4 B/op   1 allocs/op
BenchmarkAt/nrgba64-8         100000000     22.1 ns/op      8 B/op   1 allocs/op
BenchmarkAt/alpha-8           100000000     14.6 ns/op      1 B/op   1 allocs/op
BenchmarkAt/alpha16-8         200000000     6.46 ns/op      0 B/op   0 allocs/op
BenchmarkAt/gray-8            100000000     14.3 ns/op      1 B/op   1 allocs/op
BenchmarkAt/gray16-8          200000000     6.45 ns/op      0 B/op   0 allocs/op
BenchmarkAt/paletted-8        300000000     4.28 ns/op      0 B/op   0 allocs/op
BenchmarkSet/rgba-8           50000000      39.2 ns/op      8 B/op   2 allocs/op
BenchmarkSet/rgba64-8         30000000      45.8 ns/op     16 B/op   2 allocs/op
BenchmarkSet/nrgba-8          50000000      39.3 ns/op      8 B/op   2 allocs/op
BenchmarkSet/nrgba64-8        30000000      45.6 ns/op     16 B/op   2 allocs/op
BenchmarkSet/alpha-8          50000000      34.5 ns/op      2 B/op   2 allocs/op
BenchmarkSet/alpha16-8        50000000      34.9 ns/op      4 B/op   2 allocs/op
BenchmarkSet/gray-8           100000000     20.3 ns/op      1 B/op   1 allocs/op
BenchmarkSet/gray16-8         50000000      36.2 ns/op      4 B/op   2 allocs/op
BenchmarkSet/paletted-8       50000000      39.5 ns/op      1 B/op   1 allocs/op
BenchmarkRGBAAt-8             500000000     3.74 ns/op
BenchmarkRGBASetRGBA-8        300000000     4.33 ns/op
BenchmarkRGBA64At-8           300000000     5.06 ns/op
BenchmarkRGBA64SetRGBA64-8    200000000     6.61 ns/op
BenchmarkNRGBAAt-8            500000000     3.69 ns/op
BenchmarkNRGBASetNRGBA-8      300000000     4.06 ns/op
BenchmarkNRGBA64At-8          300000000     4.98 ns/op
BenchmarkNRGBA64SetNRGBA64-8  200000000     6.62 ns/op
BenchmarkAlphaAt-8            2000000000    1.43 ns/op
BenchmarkAlphaSetAlpha-8      2000000000    1.55 ns/op
BenchmarkAlpha16At-8          1000000000    2.87 ns/op
BenchmarkAlphaSetAlpha16-8    500000000     3.27 ns/op
BenchmarkGrayAt-8             2000000000    1.43 ns/op
BenchmarkGraySetGray-8        2000000000    1.55 ns/op
BenchmarkGray16At-8           1000000000    2.87 ns/op
BenchmarkGraySetGray16-8      500000000     3.14 ns/op

Updates #14884

Change-Id: I349fb214ee75f13ecbc62ac22a40e3b337648f60
Reviewed-on: https://go-review.googlesource.com/136796
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 25, 2018

Change https://golang.org/cl/137495 mentions this issue: image: optimize bounds checking for At and Set methods

Loading

@gopherbot gopherbot closed this in b57ccdf Oct 1, 2018
@golang golang locked and limited conversation to collaborators Oct 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants