You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lidavidm (via GitHub)" <gi...@apache.org> on 2023/06/20 19:42:11 UTC
[GitHub] [arrow] lidavidm opened a new pull request, #36185: GH-35960: [Java] Detect overflow in allocation
lidavidm opened a new pull request, #36185:
URL: https://github.com/apache/arrow/pull/36185
<!--
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
The Java allocator wasn't detecting overflow (which is admittedly a corner case since you can't actually allocate that many bytes), causing an unexpected exception.
### What changes are included in this PR?
Detect overflow and provide the right exception.
### Are these changes tested?
Yes
### Are there any user-facing changes?
No
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #36185: GH-35960: [Java] Detect overflow in allocation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #36185:
URL: https://github.com/apache/arrow/pull/36185#discussion_r1246538317
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java:
##########
@@ -169,9 +169,14 @@ public boolean forceAllocate(long size) {
*/
private AllocationOutcome.Status allocate(final long size, final boolean incomingUpdatePeak,
final boolean forceAllocation, AllocationOutcomeDetails details) {
- final long newLocal = locallyHeldMemory.addAndGet(size);
+ final long oldLocal = locallyHeldMemory.getAndAdd(size);
+ final long newLocal = oldLocal + size;
+ // Borrowed from Math.addExact (but avoid exception here)
Review Comment:
Exception handlers may have a performance penalty (even if not invoked), and this code is meant to be fast.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #36185: GH-35960: [Java] Detect overflow in allocation
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36185:
URL: https://github.com/apache/arrow/pull/36185#issuecomment-1619532795
Conbench analyzed the 6 benchmark runs on commit `1ab00aef`.
There were 2 benchmark results indicating a performance regression:
- Commit Run on `ursa-thinkcentre-m75q` at [2023-07-02 10:24:29Z](http://conbench.ursa.dev/compare/runs/9d00eb1500f14fe4bcd1e0aaf5efd85a...6a52b07cae0344528e43bc9d64f26724/)
- [params=<Repetition::OPTIONAL, Compression::SNAPPY>/1024/16, source=cpp-micro, suite=parquet-column-io-benchmark](http://conbench.ursa.dev/compare/benchmarks/064a125fce8c74d98000ebd5474a8965...064a151020617fac80009d1dc9b2b1ab)
- [params=<Repetition::OPTIONAL, Compression::LZ4>/1024/16, source=cpp-micro, suite=parquet-column-io-benchmark](http://conbench.ursa.dev/compare/benchmarks/064a125fcb167b8c8000ead1c04b9bab...064a15101ce87f358000c4cfde4e3c0a)
The [full Conbench report](https://github.com/apache/arrow/runs/14756682318) has more details.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] danepitkin commented on pull request #36185: GH-35960: [Java] Detect overflow in allocation
Posted by "danepitkin (via GitHub)" <gi...@apache.org>.
danepitkin commented on PR #36185:
URL: https://github.com/apache/arrow/pull/36185#issuecomment-1607785001
LGTM!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #36185: GH-35960: [Java] Detect overflow in allocation
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36185:
URL: https://github.com/apache/arrow/pull/36185#issuecomment-1599401528
:warning: GitHub issue #35960 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm merged pull request #36185: GH-35960: [Java] Detect overflow in allocation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #36185:
URL: https://github.com/apache/arrow/pull/36185
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #36185: GH-35960: [Java] Detect overflow in allocation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #36185:
URL: https://github.com/apache/arrow/pull/36185#issuecomment-1600910876
CC @davisusanibar
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] davisusanibar commented on a diff in pull request #36185: GH-35960: [Java] Detect overflow in allocation
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #36185:
URL: https://github.com/apache/arrow/pull/36185#discussion_r1246065773
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/Accountant.java:
##########
@@ -169,9 +169,14 @@ public boolean forceAllocate(long size) {
*/
private AllocationOutcome.Status allocate(final long size, final boolean incomingUpdatePeak,
final boolean forceAllocation, AllocationOutcomeDetails details) {
- final long newLocal = locallyHeldMemory.addAndGet(size);
+ final long oldLocal = locallyHeldMemory.getAndAdd(size);
+ final long newLocal = oldLocal + size;
+ // Borrowed from Math.addExact (but avoid exception here)
Review Comment:
Is it necessary to postpone exception throws?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org