ARROW-18081: [Go] Add Scalar Boolean functions #14442
Conversation
|
|
|
It looks like the current failure on the mingw64 cgo build is attributable to an existing issue with the MSYS2 mingw64 build having an issue loading libaws dlls and not something caused by the Go changes. So it is unrelated. |
go/arrow/bitutil/bitmaps.go
Outdated
| for nwords > 0 { | ||
| nwords-- | ||
| wr.PutNextWord(rdr.NextWord()) | ||
| wr.PutNextWord(modify(rdr.NextWord())) |
There was a problem hiding this comment.
Nit: since we still have to check mode explicitly in later code, looks if else is simpler here?
There was a problem hiding this comment.
simpler? yes. But i was trying to avoid an if branch in a tight loop.... I should benchmark this and see if there's any actual difference.
There was a problem hiding this comment.
Not familiar with optimization in go compiler. But won't this modify() lead to a function call? Looks no better than if as the branch is pretty easy to predict.
There was a problem hiding this comment.
I did a comparison on the benchmarks and it looks like you're right (looking a bit further into the Go opimization, I don't believe they yet fully inline functions line the modify case here).
I got this from the benchmarks where old speed is the version using an if and new speed is using the modify anonymous function approach here:
name old speed new speed delta
CopyBitmapWithOffset/32-20 202MB/s ± 5% 201MB/s ± 4% ~ (p=0.678 n=20+20)
CopyBitmapWithOffset/128-20 566MB/s ± 3% 555MB/s ± 7% -2.07% (p=0.033 n=20+20)
CopyBitmapWithOffset/1000-20 1.29GB/s ± 4% 1.23GB/s ± 3% -4.94% (p=0.000 n=20+19)
CopyBitmapWithOffset/1024-20 1.31GB/s ± 3% 1.23GB/s ± 5% -6.52% (p=0.000 n=20+20)
CopyBitmapWithOffsetBoth/32-20 174MB/s ± 3% 176MB/s ± 3% +1.19% (p=0.028 n=20+19)
CopyBitmapWithOffsetBoth/128-20 443MB/s ± 2% 442MB/s ± 3% ~ (p=0.723 n=17+20)
CopyBitmapWithOffsetBoth/1000-20 832MB/s ± 3% 823MB/s ± 3% ~ (p=0.072 n=20+18)
CopyBitmapWithOffsetBoth/1024-20 844MB/s ± 3% 829MB/s ± 3% -1.73% (p=0.001 n=20+20)
Only one case shows the function as a better option, and that's within the margin of error, so i'll change this to being an if and call it a day :)
2 participants
No description provided.