Skip to content

[SPARK-24107][CORE][followup] ChunkedByteBuffer.writeFully method has not reset the limit value - #21327

Closed
cloud-fan wants to merge 2 commits into
apache:masterfrom
cloud-fan:minor
Closed

[SPARK-24107][CORE][followup] ChunkedByteBuffer.writeFully method has not reset the limit value#21327
cloud-fan wants to merge 2 commits into
apache:masterfrom
cloud-fan:minor

Conversation

@cloud-fan

@cloud-fancloud-fan commented May 15, 2018

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

According to the discussion in #21175 , this PR proposes 2 improvements:

  1. add comments to explain why we call limit to write out ByteBuffer with slices.
  2. remove the try ... finally

How was this patch tested?

existing tests

@cloud-fan

cloud-fan commented May 15, 2018

Copy link
Copy Markdown
ContributorAuthor

cc @Manbuyun@JoshRosen@jiangxb1987

@SparkQA

SparkQA commented May 15, 2018

Copy link
Copy Markdown

Test build #90622 has finished for PR 21327 at commit cd2f0e3.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan

cloud-fan commented May 15, 2018

Copy link
Copy Markdown
ContributorAuthor

retest this please

@SparkQA

SparkQA commented May 15, 2018

Copy link
Copy Markdown

Test build #90628 has finished for PR 21327 at commit cd2f0e3.

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

advancedxy
advancedxy reviewed May 16, 2018
View reviewed changes
Comment threadcore/src/main/scala/org/apache/spark/util/io/ChunkedByteBuffer.scala Outdated
} finally {
bytes.limit(curChunkLimit)
}
// If `bytes` is an on-heap ByteBuffer, the JDK will copy it to a temporary direct

@advancedxyadvancedxyMay 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about the JDK -> the JVM?

@cloud-fancloud-fanMay 16, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the caching happens in the JDK code, not some magic inside JVM.

Comment threadcore/src/main/scala/org/apache/spark/util/io/ChunkedByteBuffer.scala Outdated
bytes.limit(curChunkLimit)
}
// If `bytes` is an on-heap ByteBuffer, the JDK will copy it to a temporary direct
// ByteBuffer when writing it out. The JDK caches one temporary buffer per thread, and we

@advancedxyadvancedxyMay 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JDK caches one temporary buffer per thread

I don't think this statement is correct. According to Util.java, the cached number of temporary direct buffer is up to IOUtil.IOV_MAX.

if the cached temp buffer gets created and freed frequently

The problem is that the varied-sized heap buffers could cause a new allocation of temporary direct buffer and free of old direct buffer if the buffer size is larger than before

@SparkQA

SparkQA commented May 16, 2018

Copy link
Copy Markdown

Test build #90686 has finished for PR 21327 at commit ad91422.

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

@SparkQA

SparkQA commented May 16, 2018

Copy link
Copy Markdown

Test build #90689 has finished for PR 21327 at commit d651d3e.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen

JoshRosen commented May 16, 2018

Copy link
Copy Markdown
Contributor

LGTM. Thanks for these changes; they really help to clarify this tricky piece of code for readers.

@cloud-fan

cloud-fan commented May 17, 2018

Copy link
Copy Markdown
ContributorAuthor

retest this please

@SparkQA

SparkQA commented May 17, 2018

Copy link
Copy Markdown

Test build #90710 has finished for PR 21327 at commit d651d3e.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya

viirya commented May 17, 2018•
edited
Loading

Copy link
Copy Markdown
Member

LGTM. It's good comment added.

@viirya

viirya commented May 17, 2018

Copy link
Copy Markdown
Member

retest this please.

@SparkQA

SparkQA commented May 17, 2018

Copy link
Copy Markdown

Test build #90715 has finished for PR 21327 at commit d651d3e.

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

@cloud-fan

cloud-fan commented May 17, 2018

Copy link
Copy Markdown
ContributorAuthor

thanks, merging to master!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@advancedxyadvancedxyadvancedxy left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cloud-fan@SparkQA@JoshRosen@viirya@advancedxy