ARROW-4104: [Java] fix a race condition in AllocationManager #3246

pravindra · 2018-12-22T13:28:22Z

No description provided.

codecov-io · 2018-12-25T14:29:21Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3246      +/-   ##
==========================================
+ Coverage    88.5%   89.27%   +0.76%     
==========================================
  Files         539      384     -155     
  Lines       72968    48981   -23987     
==========================================
- Hits        64583    43728   -20855     
+ Misses       8278     5253    -3025     
+ Partials      107        0     -107

Impacted Files	Coverage Δ
cpp/src/arrow/csv/reader.h	`0% <0%> (-100%)`	⬇️
cpp/src/arrow/csv/reader.cc	`0.52% <0%> (-90.48%)`	⬇️
cpp/src/gandiva/function_holder_registry.h	`0% <0%> (-80%)`	⬇️
cpp/src/arrow/adapters/orc/adapter.cc	`0.26% <0%> (-74.08%)`	⬇️
cpp/src/plasma/eviction_policy.cc	`59.01% <0%> (-40.99%)`	⬇️
cpp/src/plasma/events.cc	`52.5% <0%> (-35%)`	⬇️
cpp/src/arrow/csv/options.h	`66.66% <0%> (-33.34%)`	⬇️
cpp/src/plasma/thirdparty/ae/ae.c	`41.7% <0%> (-29.39%)`	⬇️
cpp/src/plasma/thirdparty/dlmalloc.c	`21.61% <0%> (-23.65%)`	⬇️
cpp/src/arrow/vendored/xxhash/xxhash.c	`51.26% <0%> (-22.34%)`	⬇️
... and 212 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 1291274...94a2015. Read the comment docs.

pravindra · 2018-12-26T20:06:14Z

@siddharthteotia @jacques-n can you please review ?

I changed the default allocation size to 3968 (from 4096) since allocating 4096 elements causes a wastage of at ~16K.
Lot of tests assume an initial allocation of 4096 or 4095 - I've modified the tests to explicitly set the initial allocation size.
fixed a race condition (bug in my earlier change)

jacques-n · 2018-12-27T03:35:27Z

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

This seems more generic than necessary. Why not make it size aware and do the allocation based on a provided value count? Also, if int[] is always length two in usage, let's make it focus on that case. For example, we shouldn't be allocating the variable data chunk in the same vector since it will have a different lifecycle.

I want to use this for both fixed-len (data+validity) and var-len vectors (offsets+validity).

For the fixed-len vectors, the data buffers and validity buffers are sized for the same number-of-elements.

For the var-len vectors, the offset buffer needs to be sized for one extra element than the bitmap.

jacques-n · 2018-12-27T03:35:58Z

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

I don't understand why this can't be generic just like the the original alloc with just valuecount *2

even if the initial allocation was not a power-of-two allocation, I'm making the realloc switch to a power-of-2. most of the code deals with that.

during realloc, we need to memcpy from original buffer, and bzero the rest.

other than the above, the code is common (allocAndSlice).

thinking further, I think this can be simplified significantly if I make both the initial allocation and the reAlloc target a multiple-of-2 allocation.

I'll need to fix a lot of tests (the allocation may be larger than what was asked for), but it's probably worth the simplicity.

jacques-n · 2018-12-27T03:36:27Z

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

Why do we have this method at all? Shouldn't this be shared with the value buffer?

This is used by an api called setNull(int index) - it just sets the validity bit to false without actually adding a data element. If this optimization isn't useful, I can switch to expand both validity/offset arrays (I preserved the old behavior).

Also, used by another api setIndexDefined(int index). I think this one isn't meaningful unless we expand both the validity and the offset arrays.

Yes, we should always only resize together. If you resized the validity buffer, you'll need to resize the data buffer ultimately anyway (when you call setValueCount). We shouldn't be ever doing a partial resize.

pravindra · 2018-12-31T06:17:19Z

@jacques-n - I've addressed the review comments. There is one change in behavior - both allocateNew and reAlloc can allocate more than the requested size, to ensure that the allocated buffer size is a power-of-2. Most of the test related changes are related to this.

can you please review now ?

jacques-n

You're commit message doesn't match your changes. Why are you making the other changes?

jacques-n · 2019-01-02T17:44:29Z

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

While we're here, we should update this message. The problem here is that we're trying to put more than X values in a record batch, which isn't allowed.

jacques-n · 2019-01-02T17:45:28Z

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

Shouldn't this be the rounded up value count?

no, the value count isn't rounded up. The buffer size is rounded up to a power-of-2 when the allocation happens, and the buffer is then used to it's full capacity.

jacques-n · 2019-01-02T17:45:53Z

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

Same comment here about exception message.

sorry, are you asking more detail in the error message, like

B bytes of memory required for capacity of C values, which exceeds the max limit of L bytes

jacques-n · 2019-01-02T17:54:21Z

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

Isn't this really allocFixedDataAndValidityBufs?

will change name

jacques-n · 2019-01-02T17:55:19Z

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

Let's return an object instead of an array.

jacques-n · 2019-01-02T17:58:04Z

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

Why not just add the pre-checks to this method rather than have in other places.

valueCount between 0 and the value count allowed for a particular type.
type width 0 to 16 (right?)

Also, shouldn't we be returning the calculated valuecount? e.g. if you size for 50 but it turns out we can fit 60 in the same memory, shouldn't we be using that as the available size/value count (for initial alloc purposes, etc)?

I'll add the pre-checks.

Also, shouldn't we be returning the calculated valuecount? e.g. if you size for 50 but it turns > out we can fit 60 in the same memory, shouldn't we be using that as the available size/value > count (for initial alloc purposes, etc)?

That's done as part of allocDataAndValidityBufs (both for initial alloc and realloc) - in this case, it will return buffers with capacity of 60. And, the vector implementations will utilize the total capacity of 60.

The api setInitialCapacity() sets the variables based on whatever the user requests. Later, during alloc/realloc, the buffers allocated can have a capacity greater than this (power-of-2 roundup).

jacques-n · 2019-01-02T17:58:31Z

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

style nit: space between cast and expression

pravindra · 2019-01-03T06:23:29Z

You're commit message doesn't match your changes. Why are you making the other changes?

My initial change reduced the heap usage only for fixed-width vectors. This change extends the same fix for variable-width vectors, and the restructuring for code re-use. I'll open a separate lira for the variable-width vectors.

pravindra · 2019-01-03T07:37:53Z

You're commit message doesn't match your changes. Why are you making the other changes?

My initial change reduced the heap usage only for fixed-width vectors. This change extends the same fix for variable-width vectors, and the restructuring for code re-use. I'll open a separate lira for the variable-width vectors.

I've opened ARROW-4147 for the rest of the change. Will use this to fix the race issue only.

pravindra · 2019-01-06T08:21:36Z

@siddharthteotia @jacques-n - this PR now only has the fix for the race condition. Can you please merge this change ?

siddharthteotia

+1

…3246)

pravindra force-pushed the batch branch from ecf133c to c6aadf5 Compare December 25, 2018 13:55

pravindra changed the title ~~ARROW-4104: [WIP] [Java] fix a race condition in AllocationManager~~ ARROW-4104: [Java] fix a race condition in AllocationManager Dec 26, 2018

jacques-n reviewed Dec 27, 2018

View reviewed changes

pravindra changed the title ~~ARROW-4104: [Java] fix a race condition in AllocationManager~~ ARROW-4104: [WIP] [Java] fix a race condition in AllocationManager Dec 27, 2018

pravindra changed the title ~~ARROW-4104: [WIP] [Java] fix a race condition in AllocationManager~~ ARROW-4104: [Java] fix a race condition in AllocationManager Dec 31, 2018

jacques-n reviewed Jan 2, 2019

View reviewed changes

ARROW-4104: [Java] fix a race condition in AllocationManager

94a2015

pravindra force-pushed the batch branch from cf00f87 to 94a2015 Compare January 3, 2019 08:44

pravindra mentioned this pull request Jan 6, 2019

ARROW-4147: [Java] reduce heap usage for varwidth vectors #3298

Merged

siddharthteotia approved these changes Jan 8, 2019

View reviewed changes

siddharthteotia merged commit 55848a3 into apache:master Jan 8, 2019

pravindra deleted the batch branch February 13, 2019 12:30

asfimport mentioned this pull request Mar 12, 2019

[Java] race in AllocationManager during release #15915

Closed

pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025

ARROW-4104: [Java] fix a race condition in AllocationManager (apache#…

9475b36

…3246)

ARROW-4104: [Java] fix a race condition in AllocationManager #3246

ARROW-4104: [Java] fix a race condition in AllocationManager #3246

Uh oh!

Conversation

pravindra commented Dec 22, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

pravindra commented Dec 26, 2018

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!

pravindra commented Dec 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

pravindra commented Jan 3, 2019

Uh oh!

pravindra commented Jan 3, 2019

Uh oh!

pravindra commented Jan 6, 2019

Uh oh!

siddharthteotia left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Dec 25, 2018 •

edited

Loading

pravindra commented Dec 31, 2018 •

edited

Loading