[SPARK-21475][Core]Revert "[SPARK-21475][CORE] Use NIO's Files API to replace FileInputStream/FileOutputStream in some critical paths" #20119

zsxwing · 2017-12-29T23:01:50Z

What changes were proposed in this pull request?

This reverts commit 5fd0294 because of a huge performance regression.
I manually fixed a minor conflict in OneForOneBlockFetcher.java.

Files.newInputStream returns sun.nio.ch.ChannelInputStream. ChannelInputStream doesn't override InputStream.skip, so it's using the default InputStream.skip which just consumes and discards data. This causes a huge performance regression when reading shuffle files.

How was this patch tested?

Jenkins

…tream/FileOutputStream in some critical paths" This reverts commit 5fd0294.

SparkQA · 2017-12-30T02:33:25Z

Test build #85532 has finished for PR 20119 at commit 401d650.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-12-30T06:08:37Z

LGTM

zsxwing · 2017-12-30T06:29:34Z

Thanks! Merging to master.

cloud-fan · 2018-01-02T05:31:47Z

let's also cc the author. @jerryshao do you know if there is a way to fix the regression?

jerryshao · 2018-01-03T01:41:00Z

Sorry I haven't checked the details, let me take a look at it. The changes I made was trying to fix memory issue for shuffle (especially external shuffle service), this issue was occurred in our prod cluster. Let me think if there's a way to fix it.

jerryshao · 2018-01-03T05:48:06Z

...n/network-common/src/main/java/org/apache/spark/network/buffer/FileSegmentManagedBuffer.java

    try {
-      is = Files.newInputStream(file.toPath());
+      is = new FileInputStream(file);
      ByteStreams.skipFully(is, offset);
      return new LimitedInputStream(is, length);


I think this two lines might be the place which suffers from skip issue, can we just only revert this place? @zsxwing @cloud-fan @gatorsmile .

zsxwing · 2018-01-03T07:28:49Z

core/src/main/scala/org/apache/spark/shuffle/IndexShuffleBlockResolver.scala

@@ -198,7 +196,7 @@ private[spark] class IndexShuffleBlockResolver(
    // find out the consolidated file, then the offset within that from our index
    val indexFile = getIndexFile(blockId.shuffleId, blockId.mapId)

-    val in = new DataInputStream(Files.newInputStream(indexFile.toPath))
+    val in = new DataInputStream(new FileInputStream(indexFile))


@jerryshao this is another place. In addition, I'm not sure if there is any compression codec using skip or not.

I also noticed sun.nio.ch.ChannelInputStream has extra synchronizeds as Files.newInputStream needs to be thread-safe. Not sure if it may cause performance regression or not.

zsxwing · 2018-01-03T07:29:45Z

...on/network-shuffle/src/main/java/org/apache/spark/network/shuffle/OneForOneBlockFetcher.java

@@ -165,7 +165,7 @@ private void failRemainingBlocks(String[] failedBlockIds, Throwable e) {

    DownloadCallback(int chunkIndex) throws IOException {
      this.targetFile = tempFileManager.createTempFile();
-      this.channel = Channels.newChannel(Files.newOutputStream(targetFile.toPath()));
+      this.channel = Channels.newChannel(new FileOutputStream(targetFile));


Seems Channels.newChannel has an optimization for FileOutputStream: http://www.grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8-b132/java/nio/channels/Channels.java#424

Here I think we can use FileChannel.open instead.

zsxwing · 2018-01-03T07:34:21Z

...n/network-common/src/main/java/org/apache/spark/network/buffer/FileSegmentManagedBuffer.java

@@ -133,7 +132,7 @@ public Object convertToNetty() throws IOException {
    if (conf.lazyFileDescriptor()) {
      return new DefaultFileRegion(file, offset, length);
    } else {
-      FileChannel fileChannel = FileChannel.open(file.toPath(), StandardOpenOption.READ);
+      FileChannel fileChannel = new FileInputStream(file).getChannel();


@jerryshao I think this is the only line that may reduce the memory pressure for external shuffle service. Right?

jerryshao · 2018-01-03T07:50:18Z

.../network-shuffle/src/main/java/org/apache/spark/network/shuffle/ShuffleIndexInformation.java

@@ -39,7 +39,7 @@ public ShuffleIndexInformation(File indexFile) throws IOException {
    offsets = buffer.asLongBuffer();
    DataInputStream dis = null;
    try {
-      dis = new DataInputStream(Files.newInputStream(indexFile.toPath()));
+      dis = new DataInputStream(new FileInputStream(indexFile));


@zsxwing also here I think it will affect external shuffle service.

jerryshao · 2018-01-03T09:14:58Z

@zsxwing maybe we only need to fix above two points related to external shuffle service, what do you think?

zsxwing · 2018-01-03T18:40:49Z

@zsxwing maybe we only need to fix above two points related to external shuffle service, what do you think?

@jerryshao sgtm. Could you submit a PR?

jerryshao · 2018-01-04T00:39:13Z

OK, I will do it.

…ternal shuffle service ## What changes were proposed in this pull request? This PR is the second attempt of #18684 , NIO's Files API doesn't override `skip` method for `InputStream`, so it will bring in performance issue (mentioned in #20119). But using `FileInputStream`/`FileOutputStream` will also bring in memory issue (https://dzone.com/articles/fileinputstream-fileoutputstream-considered-harmful), which is severe for long running external shuffle service. So here in this proposal, only fixing the external shuffle service related code. ## How was this patch tested? Existing tests. Author: jerryshao <sshao@hortonworks.com> Closes #20144 from jerryshao/SPARK-21475-v2. (cherry picked from commit 93f92c0) Signed-off-by: Shixiong Zhu <zsxwing@gmail.com>

…ternal shuffle service ## What changes were proposed in this pull request? This PR is the second attempt of #18684 , NIO's Files API doesn't override `skip` method for `InputStream`, so it will bring in performance issue (mentioned in #20119). But using `FileInputStream`/`FileOutputStream` will also bring in memory issue (https://dzone.com/articles/fileinputstream-fileoutputstream-considered-harmful), which is severe for long running external shuffle service. So here in this proposal, only fixing the external shuffle service related code. ## How was this patch tested? Existing tests. Author: jerryshao <sshao@hortonworks.com> Closes #20144 from jerryshao/SPARK-21475-v2.

Revert "[SPARK-21475][CORE] Use NIO's Files API to replace FileInputS…

401d650

…tream/FileOutputStream in some critical paths" This reverts commit 5fd0294.

asfgit closed this in 14c4a62 Dec 30, 2017

zsxwing deleted the revert-SPARK-21475 branch December 30, 2017 06:45

jerryshao reviewed Jan 3, 2018

View reviewed changes

zsxwing commented Jan 3, 2018

View reviewed changes

jerryshao reviewed Jan 3, 2018

View reviewed changes

jerryshao mentioned this pull request Jan 4, 2018

[SPARK-21475][CORE][2nd attempt] Change to use NIO's Files API for external shuffle service #20144

Closed

colinmjj mentioned this pull request Jul 22, 2022

Use NIO's Files API to replace FileInputStream/FileOutputStream in some paths apache/incubator-uniffle#65

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21475][Core]Revert "[SPARK-21475][CORE] Use NIO's Files API to replace FileInputStream/FileOutputStream in some critical paths" #20119

[SPARK-21475][Core]Revert "[SPARK-21475][CORE] Use NIO's Files API to replace FileInputStream/FileOutputStream in some critical paths" #20119

zsxwing commented Dec 29, 2017 •

edited

SparkQA commented Dec 30, 2017

gatorsmile commented Dec 30, 2017

zsxwing commented Dec 30, 2017

cloud-fan commented Jan 2, 2018

jerryshao commented Jan 3, 2018

jerryshao Jan 3, 2018

zsxwing Jan 3, 2018

jerryshao Jan 3, 2018

zsxwing Jan 3, 2018

jerryshao Jan 3, 2018

zsxwing Jan 3, 2018

jerryshao Jan 3, 2018

jerryshao commented Jan 3, 2018

zsxwing commented Jan 3, 2018

jerryshao commented Jan 4, 2018

[SPARK-21475][Core]Revert "[SPARK-21475][CORE] Use NIO's Files API to replace FileInputStream/FileOutputStream in some critical paths" #20119

[SPARK-21475][Core]Revert "[SPARK-21475][CORE] Use NIO's Files API to replace FileInputStream/FileOutputStream in some critical paths" #20119

Conversation

zsxwing commented Dec 29, 2017 • edited

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Dec 30, 2017

gatorsmile commented Dec 30, 2017

zsxwing commented Dec 30, 2017

cloud-fan commented Jan 2, 2018

jerryshao commented Jan 3, 2018

jerryshao Jan 3, 2018

Choose a reason for hiding this comment

zsxwing Jan 3, 2018

Choose a reason for hiding this comment

jerryshao Jan 3, 2018

Choose a reason for hiding this comment

zsxwing Jan 3, 2018

Choose a reason for hiding this comment

jerryshao Jan 3, 2018

Choose a reason for hiding this comment

zsxwing Jan 3, 2018

Choose a reason for hiding this comment

jerryshao Jan 3, 2018

Choose a reason for hiding this comment

jerryshao commented Jan 3, 2018

zsxwing commented Jan 3, 2018

jerryshao commented Jan 4, 2018

zsxwing commented Dec 29, 2017 •

edited