ARROW-2696: [JAVA] enhance AllocationListener with an onFailedAllocation() call #2133

vancobuca · 2018-06-11T21:25:52Z

On allocation failures, before throwing an out of memory exception, the base allocator makes a listener callback, giving the user code a change to correct its memory usage -- perhaps flush some state; perhaps clear out some caches; or perhaps trade-off some limits.

The AllocationListener has a new method added:
boolean onFailedAllocation(long size, Accountant.AllocationOutcome outcome)
with the semantics: return 'true' if the allocation can be retried (presumably after the code altered the state in some way), or 'false' if not.

If the listener asks for a retry, the allocator retries exactly once.

…o have a separate listener instance

…mplementation

siddharthteotia · 2018-06-11T21:41:06Z

cc @BryanCutler , @icexelloss , @StevenMPhillips

codecov-io · 2018-06-11T22:08:19Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2133      +/-   ##
==========================================
+ Coverage   86.39%   86.41%   +0.02%     
==========================================
  Files         242      242              
  Lines       41475    41496      +21     
==========================================
+ Hits        35832    35859      +27     
+ Misses       5643     5637       -6

Impacted Files	Coverage Δ
python/pyarrow/tests/test_table.py	`100% <0%> (ø)`	⬆️
python/pyarrow/tests/test_parquet.py	`95.54% <0%> (+0.01%)`	⬆️
python/pyarrow/tests/test_convert_pandas.py	`94.85% <0%> (+0.01%)`	⬆️
python/pyarrow/pandas_compat.py	`97.85% <0%> (+0.02%)`	⬆️
python/pyarrow/table.pxi	`69% <0%> (+0.33%)`	⬆️
cpp/src/arrow/compute/kernels/cast.cc	`89.87% <0%> (+0.51%)`	⬆️
python/pyarrow/public-api.pxi	`52.17% <0%> (+2.89%)`	⬆️

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 34890cc...4f64f96. Read the comment docs.

jacques-n

Generally looks good. Few minor comments.

jacques-n · 2018-06-12T01:05:40Z

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

-    final ChildAllocator childAllocator = new ChildAllocator(this, name, initReservation,
-        maxAllocation);
+    final ChildAllocator childAllocator = new ChildAllocator(listener,this, name, initReservation,
+                                                             maxAllocation);


weird indenting here and elsewhere.

Fixed. Please note that this passed the check style

jacques-n · 2018-06-12T01:06:27Z

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

+  // Allocation listener
+  // It counts the number of times it has been invoked, and how much memory allocation it has seen
+  // When set to 'expand on fail', it attempts to expand the associated allocator's limit
+  static class TestAllocationListener implements AllocationListener {


private final?

jacques-n · 2018-06-12T01:08:07Z

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

+    }
+
+    int getNumCalls() {
+      return numCalls;


you should be able to do this count number of calls monitoring using Jmockit's APIs rather than custom building something.

True, and in fact I had it like that initially. However, I found I was repeating code again and again in the mocks, and it was much easier to just build a custom listener.

jacques-n · 2018-06-12T01:09:22Z

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

+      // Second try, in case the listener can do something about it
+      outcome = this.allocateBytes(actualRequestSize);
+    }
    if (!outcome.isOk()) {


let's avoid checking twice in the base case.

jacques-n · 2018-06-12T01:10:10Z

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

  }

-  private BaseAllocator(
+  protected BaseAllocator(


Can we update the protected constructor/consolidate? Would rather not expose more constructors.

I'm not sure how to update/consolidate, short of removing the previous two protected constructors -- which breaks the public interface, and thus probably not the best idea. I cannot use the existing constructors since I need to support listener instances that are not common to the entire hierarchy.
Suggestions are welcome.

I think it is okay changing the constructor signature of BaseAllocator and consolidate overlap

BryanCutler

Thanks for the PR @vancobuca! Generally I feel it's better to catch the OOM exception where it's appropriate because then you have the context of what is being attempted, and take that into account for any retries. But I guess this can be helpful too in some cases, so looks ok to me.

BryanCutler · 2018-06-12T22:47:40Z

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

    assertOpen();

-    final ChildAllocator childAllocator = new ChildAllocator(this, name, initReservation,
+    final ChildAllocator childAllocator = new ChildAllocator(listener,this, name, initReservation,


missing a space between listener,this

siddharthteotia

LGTM, will merge by evening if no more comments

…ion() call (apache#2133) * Explicit listener added to newChildAllocator(), allowing each child to have a separate listener instance * AllocationListener.onFailedAlloc() method added, with default no-op implementation * Call to onFailedAlloc() implemented * Review comments * Review comment: missing space * Consolidated BaseAllocator constructors

Vanco Buca added 3 commits June 11, 2018 11:06

Explicit listener added to newChildAllocator(), allowing each child t…

d79fd98

…o have a separate listener instance

AllocationListener.onFailedAlloc() method added, with default no-op i…

e5188a8

…mplementation

Call to onFailedAlloc() implemented

6a3fc8c

jacques-n reviewed Jun 12, 2018

View reviewed changes

xhochy changed the title ~~Arrow 2696: [JAVA] enhance AllocationListener with an onFailedAllocation() call~~ ARROW-2696: [JAVA] enhance AllocationListener with an onFailedAllocation() call Jun 12, 2018

Review comments

fc97506

BryanCutler reviewed Jun 12, 2018

View reviewed changes

Vanco Buca added 2 commits June 13, 2018 09:53

Review comment: missing space

ac0b81f

Consolidated BaseAllocator constructors

4f64f96

siddharthteotia approved these changes Jun 14, 2018

View reviewed changes

siddharthteotia merged commit 392fd02 into apache:master Jun 15, 2018

Uh oh!

ARROW-2696: [JAVA] enhance AllocationListener with an onFailedAllocation() call #2133

ARROW-2696: [JAVA] enhance AllocationListener with an onFailedAllocation() call #2133

Uh oh!

Conversation

vancobuca commented Jun 11, 2018

Uh oh!

siddharthteotia commented Jun 11, 2018

Uh oh!

codecov-io commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jacques-n 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!

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!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthteotia left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jun 11, 2018 •

edited

Loading