[#596] feat(netty): Use off heap memory to read HDFS data #806

jerqi · 2023-04-10T06:10:38Z

What changes were proposed in this pull request?

Use off heap memory to read HDFS data
remove some unused code
(to do: use off heap memory to read HDFS index data)

Why are the changes needed?

Fix: #596

Does this PR introduce any user-facing change?

Yes, add the document.

How was this patch tested?

Pass origin tests.

codecov-commenter · 2023-04-10T08:13:24Z

Codecov Report

Merging #806 (d198fb5) into master (c9abe9a) will increase coverage by 1.23%.
The diff coverage is 35.23%.

@@             Coverage Diff              @@
##             master     #806      +/-   ##
============================================
+ Coverage     57.63%   58.87%   +1.23%     
- Complexity     2058     2062       +4     
============================================
  Files           306      292      -14     
  Lines         14871    12976    -1895     
  Branches       1221     1232      +11     
============================================
- Hits           8571     7639     -932     
+ Misses         5808     4900     -908     
+ Partials        492      437      -55

Impacted Files	Coverage Δ
...pache/hadoop/mapreduce/task/reduce/RssShuffle.java	`0.00% <ø> (ø)`
...e/uniffle/client/factory/ShuffleClientFactory.java	`0.00% <0.00%> (ø)`
...client/request/CreateShuffleReadClientRequest.java	`0.00% <0.00%> (ø)`
.../java/org/apache/uniffle/common/util/RssUtils.java	`57.77% <0.00%> (-1.32%)`	⬇️
...uniffle/storage/factory/ShuffleHandlerFactory.java	`0.00% <0.00%> (ø)`
...e/uniffle/storage/handler/impl/HdfsFileReader.java	`48.07% <0.00%> (-35.26%)`	⬇️
.../uniffle/storage/handler/impl/LocalFileReader.java	`50.00% <0.00%> (-3.34%)`	⬇️
...orage/request/CreateShuffleReadHandlerRequest.java	`0.00% <0.00%> (ø)`
...ache/uniffle/storage/util/ShuffleStorageUtils.java	`71.08% <ø> (+3.26%)`	⬆️
...e/storage/handler/impl/HdfsShuffleReadHandler.java	`51.35% <25.00%> (-4.90%)`	⬇️
... and 6 more

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kaijchen · 2023-04-10T08:32:23Z

client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java

-    byteBufInputStream = new ByteBufInputStream(Unpooled.wrappedBuffer(data.array(), data.position(), size), true);
+    // Uncompressed data is released in this class, Compressed data is release in the class ShuffleReadClientImpl
+    // So if codec is null, we don't release the data when the stream is closed
+    byteBufInputStream = new ByteBufInputStream(Unpooled.wrappedBuffer(data), codec != null);


Is it possible to unify where the buffer is released?

It seems difficult. I don't have good idea.

client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java

advancedxy · 2023-04-11T02:26:28Z

I believe the off-heap read should be optional and configurable.

The reader happens in the client side, which mostly are spark clients. Spark applications doesn't enable off-heap management by default. If this is mandatory, it would require users to modify spark configurations to avoid direct memory out of memory.

jerqi · 2023-04-11T02:36:47Z

I believe the off-heap read should be optional and configurable.

The reader happens in the client side, which mostly are spark clients. Spark applications doesn't enable off-heap management by default. If this is mandatory, it would require users to modify spark configurations to avoid direct memory out of memory.

We have controlled the size of data which we read. It is usually 32MB, it won't occupy too much off heap memory. If we add a config option for this feature. We will suffered the more GC problems when we use default config option and we need to mantain heap memory and off heap memory mode at the same time. It will burden the pressure of code maintain.

client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java

common/src/main/java/org/apache/uniffle/common/ShuffleDataResult.java

common/src/main/java/org/apache/uniffle/common/util/RssUtils.java

advancedxy · 2023-04-11T07:02:39Z

We have controlled the size of data which we read. It is usually 32MB, it won't occupy too much off heap memory. If we add a config option for this feature. We will suffered the more GC problems when we use default config option and we need to mantain heap memory and off heap memory mode at the same time. It will burden the pressure of code maintain.

Do you have any cases that the client is suffered from GC problems and is especially related to HDFS data read code path?

It's just that normally no other system would to support read hdfs via off-heap bytebuffer specially?

As for code maintenance, it's the bill that we have to pay.

jerqi · 2023-04-11T07:15:43Z

We have controlled the size of data which we read. It is usually 32MB, it won't occupy too much off heap memory. If we add a config option for this feature. We will suffered the more GC problems when we use default config option and we need to mantain heap memory and off heap memory mode at the same time. It will burden the pressure of code maintain.

Do you have any cases that the client is suffered from GC problems and is especially related to HDFS data read code path?

It's just that normally no other system would to support read hdfs via off-heap bytebuffer specially?

As for code maintenance, it's the bill that we have to pay.

Our client's GC time is longer than origin Spark when we run the TPCDS
Some new systems will be used direct buffer because they want to use vector read.
It may be hard to use if we need modify the configuration. In origin code, this code used direct memory, it's changed by @zuston . It's ok for us to use direct memory. And the memory manager is the concept of Spark, We don't have memory manager in the shuffle client.

zuston · 2023-04-11T07:51:15Z

Our client's GC time is longer than origin Spark when we run the TPCDS

I guess this is caused by the too many small objects. Could we use the resident shareable memory of spark to avoid memory allocation?

…HdfsShuffleReadHandler.java Co-authored-by: advancedxy <xianjin@apache.org>

jerqi · 2023-04-12T06:01:35Z

@advancedxy All comments are addressed.

...ark-common/src/test/java/org/apache/uniffle/test/RepartitionWithHdfsMultiStorageRssTest.java

advancedxy

Generally lgtm, left minor comments

storage/src/main/java/org/apache/uniffle/storage/handler/impl/HdfsFileReader.java

client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java

advancedxy

LGTM, thanks for your working

### What changes were proposed in this pull request? We support to read off heap data in the #806, we support to read index in this pr. ### Why are the changes needed? #596 follow up pr ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? GA passed.

jerqi added 3 commits April 10, 2023 14:07

[apache#596] Use off heap memory to read HDFS data

e302695

fix

8ae5ded

fix

5a55ab1

jerqi marked this pull request as draft April 10, 2023 06:31

jerqi added 3 commits April 10, 2023 14:42

fix

cf2c023

fix

7a5e968

fix

40fbc88

jerqi changed the title ~~[#596] Use off heap memory to read HDFS data~~ [#596] feat(netty): Use off heap memory to read HDFS data Apr 10, 2023

jerqi added 5 commits April 10, 2023 15:05

fix

d6f4498

fix

0bd5ca7

fix

578bb49

fix

da5ceea

fix

f451194

jerqi marked this pull request as ready for review April 10, 2023 08:25

jerqi requested review from advancedxy, kaijchen and zuston and removed request for kaijchen April 10, 2023 08:26

kaijchen reviewed Apr 10, 2023

View reviewed changes

fix

78ef7d6

zuston reviewed Apr 11, 2023

View reviewed changes

client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java Outdated Show resolved Hide resolved

jerqi added 2 commits April 11, 2023 10:26

fix

9ce762e

fix

590546a

advancedxy reviewed Apr 11, 2023

View reviewed changes

jerqi and others added 13 commits April 12, 2023 09:54

Update storage/src/main/java/org/apache/uniffle/storage/handler/impl/…

7914648

…HdfsShuffleReadHandler.java Co-authored-by: advancedxy <xianjin@apache.org>

fix

4d37ea6

fix

9fc2897

fix

f20a874

fix

74e13c7

fix

4f9a5be

fix

d690521

fix

6fd9692

fix

32a7471

fix ut

1046c53

fix

742fff9

fix

e5c15d2

fix

37faa8e

jerqi requested a review from advancedxy April 12, 2023 06:11

jerqi added 2 commits April 12, 2023 14:17

add doc

8feb03a

fix error

016907c

smallzhongfeng reviewed Apr 12, 2023

View reviewed changes

...ark-common/src/test/java/org/apache/uniffle/test/RepartitionWithHdfsMultiStorageRssTest.java Show resolved Hide resolved

jerqi mentioned this pull request Apr 12, 2023

[Improvement] Add parameter test for repartitionTest #817

Open

Merge remote-tracking branch 'upstream/master' into reader-off

77d8afb

advancedxy reviewed Apr 12, 2023

View reviewed changes

address comments

5fab041

jerqi requested a review from advancedxy April 12, 2023 11:22

advancedxy reviewed Apr 12, 2023

View reviewed changes

client-spark/common/src/main/java/org/apache/spark/shuffle/reader/RssShuffleDataIterator.java Show resolved Hide resolved

fix comment

d198fb5

jerqi requested a review from advancedxy April 13, 2023 01:38

advancedxy approved these changes Apr 13, 2023

View reviewed changes

jerqi merged commit c6cde5d into apache:master Apr 13, 2023
23 checks passed

jerqi mentioned this pull request May 8, 2023

[#596][FOLLOWUP] Index data support offheap read #852

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#596] feat(netty): Use off heap memory to read HDFS data #806

[#596] feat(netty): Use off heap memory to read HDFS data #806

jerqi commented Apr 10, 2023 •

edited

codecov-commenter commented Apr 10, 2023 •

edited

kaijchen Apr 10, 2023

jerqi Apr 10, 2023

advancedxy commented Apr 11, 2023

jerqi commented Apr 11, 2023

advancedxy commented Apr 11, 2023

jerqi commented Apr 11, 2023

zuston commented Apr 11, 2023 •

edited

jerqi commented Apr 12, 2023 •

edited

advancedxy left a comment

advancedxy left a comment

[#596] feat(netty): Use off heap memory to read HDFS data #806

[#596] feat(netty): Use off heap memory to read HDFS data #806

Conversation

jerqi commented Apr 10, 2023 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov-commenter commented Apr 10, 2023 • edited

Codecov Report

kaijchen Apr 10, 2023

Choose a reason for hiding this comment

jerqi Apr 10, 2023

Choose a reason for hiding this comment

advancedxy commented Apr 11, 2023

jerqi commented Apr 11, 2023

advancedxy commented Apr 11, 2023

jerqi commented Apr 11, 2023

zuston commented Apr 11, 2023 • edited

jerqi commented Apr 12, 2023 • edited

advancedxy left a comment

Choose a reason for hiding this comment

advancedxy left a comment

Choose a reason for hiding this comment

jerqi commented Apr 10, 2023 •

edited

codecov-commenter commented Apr 10, 2023 •

edited

zuston commented Apr 11, 2023 •

edited

jerqi commented Apr 12, 2023 •

edited