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

GH-41541: [Go][Parquet] More fixes for writer performance regression #42003

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

nontan-pixiv
Copy link
Contributor

@nontan-pixiv nontan-pixiv commented Jun 6, 2024

Rationale for this change

This PR is complementary to #41638 .

The prior PR reduces reallocations in PooledBufferWriter. However the problematic formula it addressed is still used in other functions.

In addition to this, (*PooledBufferWriter).Reserve() simply doubles the capacity of buffers regardless of its argument nbytes. This may result in excessive allocations in some cases.

What changes are included in this PR?

  • Applied the fixed formula to (*BufferWriter).Reserve().
  • Updated the new capacity passed to (*memory.Buffer).Reserve().
    • Now using bitutil.NextPowerOf2(b.pos + nbytes) to avoid reallocations when adding nbytes.
  • Replaced math.Max with utils.Max in (*bufferWriteSeeker).Reserve() to avoid unnecessary type conversions.

Are these changes tested?

Yes. The following commands pass.

$ export PARQUET_TEST_DATA=$PWD/cpp/submodules/parquet-testing/data
$ (cd go && go test ./...)

Are there any user-facing changes?

No, but it may reduce the number of allocations and improve the throughput.

Before:

$ go test -test.run='^$' -test.bench='^BenchmarkWriteColumn$' -benchmem ./parquet/pqarrow/...
goos: linux
goarch: arm64
pkg: github.com/apache/arrow/go/v17/parquet/pqarrow
BenchmarkWriteColumn/int32_not_nullable-10                  1190           1016705 ns/op        4125.39 MB/s     5443579 B/op        240 allocs/op
BenchmarkWriteColumn/int32_nullable-10                        52          24780561 ns/op         169.26 MB/s    12048944 B/op        249 allocs/op
BenchmarkWriteColumn/int64_not_nullable-10                   632           1717090 ns/op        4885.36 MB/s     5445954 B/op        265 allocs/op
BenchmarkWriteColumn/int64_nullable-10                        51          22949770 ns/op         365.52 MB/s    12209860 B/op        262 allocs/op
BenchmarkWriteColumn/float32_not_nullable-10                 519           2234718 ns/op        1876.88 MB/s     5452627 B/op       1263 allocs/op
BenchmarkWriteColumn/float32_nullable-10                      56          23423793 ns/op         179.06 MB/s    12057540 B/op       1272 allocs/op
BenchmarkWriteColumn/float64_not_nullable-10                 416           2761247 ns/op        3037.98 MB/s     5507068 B/op       1292 allocs/op
BenchmarkWriteColumn/float64_nullable-10                      51          25767881 ns/op         325.55 MB/s    12059614 B/op       1285 allocs/op
PASS
ok      github.com/apache/arrow/go/v17/parquet/pqarrow  10.592s

After:

$ go test -test.run='^$' -test.bench='^BenchmarkWriteColumn$' -benchmem ./parquet/pqarrow/...
goos: linux
goarch: arm64
pkg: github.com/apache/arrow/go/v17/parquet/pqarrow
BenchmarkWriteColumn/int32_not_nullable-10                  1196            959528 ns/op        4371.22 MB/s     5420349 B/op        238 allocs/op
BenchmarkWriteColumn/int32_nullable-10                        51          23017598 ns/op         182.22 MB/s    14138480 B/op        248 allocs/op
BenchmarkWriteColumn/int64_not_nullable-10                   690           1671710 ns/op        5017.98 MB/s     5419878 B/op        263 allocs/op
BenchmarkWriteColumn/int64_nullable-10                        50          23196051 ns/op         361.64 MB/s    13728465 B/op        261 allocs/op
BenchmarkWriteColumn/float32_not_nullable-10                 540           2185075 ns/op        1919.52 MB/s     5459392 B/op       1261 allocs/op
BenchmarkWriteColumn/float32_nullable-10                      54          21796783 ns/op         192.43 MB/s    14150622 B/op       1271 allocs/op
BenchmarkWriteColumn/float64_not_nullable-10                 418           2708292 ns/op        3097.38 MB/s     5455095 B/op       1290 allocs/op
BenchmarkWriteColumn/float64_nullable-10                      51          22174952 ns/op         378.29 MB/s    14142791 B/op       1283 allocs/op
PASS
ok      github.com/apache/arrow/go/v17/parquet/pqarrow  10.210s

Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! LGTM assuming no CI issues

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Jun 7, 2024
@zeroshade zeroshade merged commit c401348 into apache:main Jul 10, 2024
25 checks passed
@zeroshade
Copy link
Member

Sorry for the long delay on getting this merged, it dropped off my radar for a bit

Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit c401348.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

@raulcd
Copy link
Member

raulcd commented Jul 11, 2024

Hi, I am in the midst of a release and I was confused when seeing this issue being tagged as 17.0.0. Please do not reuse GitHub issues in the future. This should have been a new GitHub issue instead of reusing the original one.

raulcd pushed a commit that referenced this pull request Jul 11, 2024
…42003)

### Rationale for this change

<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->

This PR is complementary to #41638 .

The prior PR reduces reallocations in `PooledBufferWriter`. However the
problematic formula it addressed is still used in other functions.

In addition to this, `(*PooledBufferWriter).Reserve()` simply doubles
the capacity of buffers regardless of its argument `nbytes`. This may
result in excessive allocations in some cases.

### What changes are included in this PR?

<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->

- Applied the fixed formula to `(*BufferWriter).Reserve()`.
- Updated the new capacity passed to `(*memory.Buffer).Reserve()`.
- Now using `bitutil.NextPowerOf2(b.pos + nbytes)` to avoid
reallocations when adding `nbytes`.
- Replaced `math.Max` with `utils.Max` in
`(*bufferWriteSeeker).Reserve()` to avoid unnecessary type conversions.

### Are these changes tested?

<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code

If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
-->

Yes. The following commands pass.

```
$ export PARQUET_TEST_DATA=$PWD/cpp/submodules/parquet-testing/data
$ (cd go && go test ./...)
```

### Are there any user-facing changes?

<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->

No, but it may reduce the number of allocations and improve the
throughput.

Before:

```
$ go test -test.run='^$' -test.bench='^BenchmarkWriteColumn$' -benchmem ./parquet/pqarrow/...
goos: linux
goarch: arm64
pkg: github.com/apache/arrow/go/v17/parquet/pqarrow
BenchmarkWriteColumn/int32_not_nullable-10                  1190           1016705 ns/op        4125.39 MB/s     5443579 B/op        240 allocs/op
BenchmarkWriteColumn/int32_nullable-10                        52          24780561 ns/op         169.26 MB/s    12048944 B/op        249 allocs/op
BenchmarkWriteColumn/int64_not_nullable-10                   632           1717090 ns/op        4885.36 MB/s     5445954 B/op        265 allocs/op
BenchmarkWriteColumn/int64_nullable-10                        51          22949770 ns/op         365.52 MB/s    12209860 B/op        262 allocs/op
BenchmarkWriteColumn/float32_not_nullable-10                 519           2234718 ns/op        1876.88 MB/s     5452627 B/op       1263 allocs/op
BenchmarkWriteColumn/float32_nullable-10                      56          23423793 ns/op         179.06 MB/s    12057540 B/op       1272 allocs/op
BenchmarkWriteColumn/float64_not_nullable-10                 416           2761247 ns/op        3037.98 MB/s     5507068 B/op       1292 allocs/op
BenchmarkWriteColumn/float64_nullable-10                      51          25767881 ns/op         325.55 MB/s    12059614 B/op       1285 allocs/op
PASS
ok      github.com/apache/arrow/go/v17/parquet/pqarrow  10.592s
```

After:

```
$ go test -test.run='^$' -test.bench='^BenchmarkWriteColumn$' -benchmem ./parquet/pqarrow/...
goos: linux
goarch: arm64
pkg: github.com/apache/arrow/go/v17/parquet/pqarrow
BenchmarkWriteColumn/int32_not_nullable-10                  1196            959528 ns/op        4371.22 MB/s     5420349 B/op        238 allocs/op
BenchmarkWriteColumn/int32_nullable-10                        51          23017598 ns/op         182.22 MB/s    14138480 B/op        248 allocs/op
BenchmarkWriteColumn/int64_not_nullable-10                   690           1671710 ns/op        5017.98 MB/s     5419878 B/op        263 allocs/op
BenchmarkWriteColumn/int64_nullable-10                        50          23196051 ns/op         361.64 MB/s    13728465 B/op        261 allocs/op
BenchmarkWriteColumn/float32_not_nullable-10                 540           2185075 ns/op        1919.52 MB/s     5459392 B/op       1261 allocs/op
BenchmarkWriteColumn/float32_nullable-10                      54          21796783 ns/op         192.43 MB/s    14150622 B/op       1271 allocs/op
BenchmarkWriteColumn/float64_not_nullable-10                 418           2708292 ns/op        3097.38 MB/s     5455095 B/op       1290 allocs/op
BenchmarkWriteColumn/float64_nullable-10                      51          22174952 ns/op         378.29 MB/s    14142791 B/op       1283 allocs/op
PASS
ok      github.com/apache/arrow/go/v17/parquet/pqarrow  10.210s
```

<!--
If there are any breaking changes to public APIs, please uncomment the
line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->

<!--
Please uncomment the line below (and provide explanation) if the changes
fix either (a) a security vulnerability, (b) a bug that caused incorrect
or invalid data to be produced, or (c) a bug that causes a crash (even
when the API contract is upheld). We use this to highlight fixes to
issues that may affect users without their knowledge. For this reason,
fixing bugs that cause errors don't count, since those are usually
obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
* GitHub Issue: #41541
@zeroshade
Copy link
Member

@raulcd I should have noticed that had happened, my fault here. Sorry

@nontan-pixiv nontan-pixiv deleted the fix-go-parquet-realloc branch July 17, 2024 23:01
@nontan-pixiv
Copy link
Contributor Author

Please do not reuse GitHub issues in the future. This should have been a new GitHub issue instead of reusing the original one.

Sorry for the confusion. I will create a new one for the next time.

@raulcd
Copy link
Member

raulcd commented Jul 18, 2024

@nontan-pixiv no worries, it was just confusing during the release process

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants