ARROW-4086: [Java] Add apis to debug memory alloc failures by pravindra · Pull Request #4369 · apache/arrow

pravindra · 2019-05-22T12:27:14Z

On failures, capture state for each allocator in chain and add the
details to the exception.
add APIs to get parent/child allocators
Do not mask the original exception when allocating from vectors

pravindra · 2019-05-27T02:58:24Z

@praveenbingo can you please review ?

emkornfield · 2019-05-27T04:42:12Z

java/memory/src/main/java/org/apache/arrow/memory/Accountant.java

small nit, would it make sense to construct the OOM with reservation and outcome instead of string formatting here?

this one is slightly different - the allocation is failing when allocating the reservation. The other failures happen when allocating a buffer. Also, we throw the same exception if the allocator's metrics satisfy the request, but netty doesn't have available memory (can happen due to fragmentation).

so, i think it's better to leave the string be constructed as per the context.

emkornfield · 2019-05-27T04:43:39Z

java/memory/src/main/java/org/apache/arrow/memory/Accountant.java

you can avoid the second comparison (compiler might optimize it away anyways) if you return the new allocation inside the if block above and return SUCCESS_INSANCE here.

emkornfield · 2019-05-27T04:44:27Z

java/memory/src/main/java/org/apache/arrow/memory/Accountant.java

nit: what do you think about adding /foo=/true, /bar=/ false it would make this more readable outside and IDE

i made the change but it looks a bit ugly inside an IDE - intellij shows me the field name twice now.

emkornfield · 2019-05-27T04:46:16Z

java/memory/src/main/java/org/apache/arrow/memory/Accountant.java

nit: nested ternary operators are a little hard to read.

emkornfield · 2019-05-27T04:47:37Z

java/memory/src/main/java/org/apache/arrow/memory/AllocationOutcome.java

do we have any standard policy for changing the public API in Java?

None so far, the changes i've seen so far are backward incompatible..they should not be though :)

emkornfield · 2019-05-27T05:03:51Z

java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java

assert something about the OOM message?

emkornfield · 2019-05-27T05:05:32Z

java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java

it might be clearer to call this something like failureAtParentLimit

renamed to failureAtParentLimitOutcomeDetails

emkornfield · 2019-05-27T05:05:49Z

java/memory/src/test/java/org/apache/arrow/memory/TestBaseAllocator.java

failureAtRootLimit?

renamed to failureAtRootLimitOutcomeDetails

emkornfield · 2019-05-27T05:06:26Z

java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthVector.java

why isn't clear necessary anymore?

because allocateNew is already doing the same. please see line 410..

emkornfield · 2019-05-27T05:08:48Z

java/memory/src/main/java/org/apache/arrow/memory/Accountant.java

it seems like it might be cleaner to overload. Is there a reason not to so?

added an overloaded fn - but i'm not sure that's what you meant. please take a look.

praveenbingo · 2019-05-27T11:41:57Z

java/memory/src/main/java/org/apache/arrow/memory/Accountant.java

praveenbingo · 2019-05-27T11:53:07Z

java/memory/src/main/java/org/apache/arrow/memory/AllocationOutcome.java

None so far, the changes i've seen so far are backward incompatible..they should not be though :)

praveenbingo · 2019-05-27T11:53:24Z

java/memory/src/main/java/org/apache/arrow/memory/AllocationOutcome.java

Use optional instead ?

praveenbingo · 2019-05-27T12:10:28Z

java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java

is there no concurrent identity hash map..might prefer that since it will prevent future bugs..

(or) might create a synchronized map that wraps the identity map..

done - created a synchronized map wrapper over the identity map

praveenbingo · 2019-05-27T13:15:48Z

java/vector/src/main/java/org/apache/arrow/vector/BaseFixedWidthVector.java

nice work consolidating this..

praveenbingo · 2019-05-27T13:16:43Z

java/vector/src/main/java/org/apache/arrow/vector/BaseVariableWidthVector.java

because allocateNew is already doing the same. please see line 410..

codecov-io · 2019-05-28T05:50:58Z

Codecov Report

Merging #4369 into master will increase coverage by 1.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4369      +/-   ##
==========================================
+ Coverage   88.32%   89.38%   +1.06%     
==========================================
  Files         781      639     -142     
  Lines       98540    88864    -9676     
  Branches     1251        0    -1251     
==========================================
- Hits        87031    79434    -7597     
+ Misses      11273     9430    -1843     
+ Partials      236        0     -236

Impacted Files	Coverage Δ
cpp/src/parquet/schema-test.cc	`90.63% <0%> (-8.86%)`	⬇️
cpp/src/arrow/flight/client.cc	`92.27% <0%> (-2.03%)`	⬇️
cpp/src/parquet/column_scanner.h	`95.6% <0%> (-0.87%)`	⬇️
cpp/src/arrow/flight/server.cc	`90.06% <0%> (-0.78%)`	⬇️
python/pyarrow/_parquet.pyx	`90.85% <0%> (-0.35%)`	⬇️
cpp/src/arrow/flight/internal.cc	`91.3% <0%> (-0.23%)`	⬇️
cpp/src/parquet/reader-test.cc	`100% <0%> (ø)`	⬆️
python/pyarrow/cuda.py	`100% <0%> (ø)`	⬆️
python/pyarrow/ipc.py	`100% <0%> (ø)`	⬆️
cpp/src/arrow/flight/test-util.h	`100% <0%> (ø)`	⬆️
... and 201 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c4d433...7d88ccb. Read the comment docs.

praveenbingo · 2019-05-28T06:13:40Z

@emkornfield +1 LGTM..Please merge if you are ok with the changes.

emkornfield

Two minor comments but otherwise looks good.

emkornfield · 2019-05-29T05:23:20Z

java/memory/src/main/java/org/apache/arrow/memory/BaseAllocator.java

Simpler to just do: new HashSet<>(childAlloactors.keySet()) https://docs.oracle.com/javase/7/docs/api/java/util/HashSet.html#HashSet(java.util.Collection)

emkornfield · 2019-05-29T05:25:54Z

java/memory/src/main/java/org/apache/arrow/memory/AllocationOutcome.java

This needs a javadoc or CI will fail.

- On failures, capture state for each allocator in chain and add the details to the exception. - add APIs to get parent/child allocators - Do not mask the original exception when allocating from vectors

- On failures, capture state for each allocator in chain and add the details to the exception. - add APIs to get parent/child allocators - Do not mask the original exception when allocating from vectors Author: Pindikura Ravindra <ravindra@dremio.com> Closes apache#4369 from pravindra/memdebug and squashes the following commits: 92a269b <Pindikura Ravindra> ARROW-4086 : Add apis to debug memory alloc failures

pravindra changed the title ~~ARROW-4086 : [WIP][Java] Add apis to debug memory alloc failures~~ ARROW-4086 :[Java] Add apis to debug memory alloc failures May 24, 2019

pravindra requested a review from siddharthteotia May 27, 2019 02:57

emkornfield reviewed May 27, 2019

View reviewed changes

praveenbingo reviewed May 27, 2019

View reviewed changes

kou changed the title ~~ARROW-4086 :[Java] Add apis to debug memory alloc failures~~ ARROW-4086: [Java] Add apis to debug memory alloc failures May 27, 2019

emkornfield approved these changes May 29, 2019

View reviewed changes

ARROW-4086 : [Java] Add apis to debug memory alloc failures

92a269b

- On failures, capture state for each allocator in chain and add the details to the exception. - add APIs to get parent/child allocators - Do not mask the original exception when allocating from vectors

pravindra force-pushed the memdebug branch from 7d88ccb to 92a269b Compare May 29, 2019 07:23

pravindra closed this in 2ea7350 May 29, 2019

asfimport mentioned this pull request May 29, 2019

[Java] Add apis to debug alloc failures #20679

Closed

Conversation

pravindra commented May 22, 2019

Uh oh!

pravindra commented May 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

praveenbingo commented May 28, 2019

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

codecov-io commented May 28, 2019 •

edited

Loading