zeroshade opened a new pull request, #41638:
URL: https://github.com/apache/arrow/pull/41638
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's
Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing
Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor
PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes).
Could you open an issue for this pull request on GitHub?
https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the
[Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.)
of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
A performance regression was reported for the parquet writer since v14.
Profiling revealed excessive allocations. This was due to us always adding the
current offset to the current capacity when reserving, resulting in Reserve
always performing a reallocate even when it didn't need to.
<!--
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.
-->
### What changes are included in this PR?
`PooledBufferWriter` should only pass `nbytes` to the `Reserve` call, not
`byteoffset + nbytes`. `BitWriter` should not be adding `b.offset` to the
capacity when determining the new capacity.
<!--
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.
-->
### Are these changes tested?
Yes.
<!--
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)?
-->
### Are there any user-facing changes?
No, only performance changes:
Before:
```shell
goos: linux
goarch: amd64
pkg: github.com/apache/arrow/go/v17/parquet/pqarrow
cpu: 12th Gen Intel(R) Core(TM) i7-12700H
BenchmarkWriteColumn/int32_not_nullable-20 514
2127175 ns/op 1971.77 MB/s 5425676 B/op 239 allocs/op
BenchmarkWriteColumn/int32_nullable-20 31
467352621 ns/op 8.97 MB/s 2210271923 B/op2350 allocs/op
BenchmarkWriteColumn/int64_not_nullable-20 326
4132204 ns/op 2030.06 MB/s 5442976 B/op 265 allocs/op
BenchmarkWriteColumn/int64_nullable-20 33
432764687 ns/op 19.38 MB/s 2100068812 B/op2384 allocs/op
BenchmarkWriteColumn/float32_not_nullable-20 334
3540566 ns/op 1184.64 MB/s 5453079 B/op 1263 allocs/op
BenchmarkWriteColumn/float32_nullable-20 6
492103646 ns/op 8.52 MB/s 2283305841 B/op3371 allocs/op
BenchmarkWriteColumn/float64_not_nullable-20 241
4783268 ns/op 1753.74 MB/s 5498759 B/op 1292 allocs/op
BenchmarkWriteColumn/float64_nullable-20 4
369619096 ns/op 22.70 MB/s 1725354454 B/op3401 allocs/op
PASS
ok github.com/apache/arrow/go/v17/parquet/pqarrow 40.862s
```
After:
```shell
goos: linux
goarch: amd64
pkg: github.com/apache/arrow/go/v17/parquet/pqarrow
cpu: 12th Gen Intel(R) Core(TM) i7-12700H
BenchmarkWriteColumn/int32_not_nullable-20 500
2136823 ns/op 1962.87 MB/s 5410591 B/op 240 allocs/op
BenchmarkWriteColumn/int32_nullable-20 48
26604880 ns/op 157.65 MB/s 12053510 B/op 250 allocs/op
BenchmarkWriteColumn/int64_not_nullable-20 340
3530509 ns/op 2376.03 MB/s 5439578 B/op 265 allocs/op
BenchmarkWriteColumn/int64_nullable-20 44
27387334 ns/op 306.30 MB/s 11870305 B/op 260 allocs/op
BenchmarkWriteColumn/float32_not_nullable-20 316
3479312 ns/op 1205.50 MB/s 5456685 B/op 1263 allocs/op
BenchmarkWriteColumn/float32_nullable-20 50
25910872 ns/op 161.87 MB/s 12054582 B/op 1271 allocs/op
BenchmarkWriteColumn/float64_not_nullable-20 249
4769664 ns/op 1758.74 MB/s 5486020 B/op 1292 allocs/op
BenchmarkWriteColumn/float64_nullable-20 51
25496256 ns/op 329.01 MB/s 12140753 B/op 1284 allocs/op
PASS
ok github.com/apache/arrow/go/v17/parquet/pqarrow 11.492s
```
All of the nullable column cases average around a 16x-17x performance
improvement.
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->
<!--
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".** -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
