Skip to content

Conversation

@runzhiwang
Copy link
Contributor

@runzhiwang runzhiwang commented Apr 20, 2020

What changes were proposed in this pull request?

What's the problem ?

Use dd to read 1000M object from ozone bucket mounted by goofys, it cost about 470 seconds, i.e. 2.2M/s, which is too slow.
image

I also use tcpdump to capture the packet when I read 200M object. As the image shows, the 1st GET request cost about 1 second, but the 10th GET request cost about 22 seconds. The GET request become slower and slower.
image

What's the reason ?
When read 1000M object, there are 50 GET requests, each GET request read 20M. When do GET, the stack is: IOUtils::copyLarge -> IOUtils::skipFully -> IOUtils::skip -> InputStream::read.

It means, the 50th GET request which should read 980M-1000M, but to skip 0-980M, it also InputStream::read 0-980M. So the 1st GET request read 0-20M, the 2nd GET request read 0-40M, the 3rd GET request read 0-60M, ..., the 50th GET request read 0-1000M. So the GET request from 1st-50th become slower and slower.

You can also refer IO-203 and IO-355 why IOUtils implement skip by read rather than real skip, e.g. seek.
image

How to improve ?
Copy IOUtils::copyLarge to S3WrapperInputStream, and replace IOUtils::skipFully with S3WrapperInputStream::seek. No other changes of IOUtils::copyLarge.
After improving, read 1000M object cost 4.79 seconds, i.e. 219M/s, about 100 times faster.
image

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3223

How was this patch tested?

Existed UT and IT.

@runzhiwang runzhiwang force-pushed the s3g-seek branch 2 times, most recently from ced02e6 to 2388d62 Compare April 20, 2020 05:59
Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

Thank You @runzhiwang for the analysis and great improvement in read performance.

I see there is one more usage of IOUtils.copyLarge in createMultipartKey, can we update over there also with a similar change.

L534:

            if (range != null) {
              RangeHeader rangeHeader =
                  RangeHeaderParserUtil.parseRangeHeader(range, 0);
              IOUtils.copyLarge(sourceObject, ozoneOutputStream,
                  rangeHeader.getStartOffset(),
                  rangeHeader.getEndOffset() - rangeHeader.getStartOffset());

            }

And also can we add test for S3WrapperInputStream which tests a new method with multiple seek values.

@runzhiwang
Copy link
Contributor Author

Thank You @runzhiwang for the analysis and great improvement in read performance.

I see there is one more usage of IOUtils.copyLarge in createMultipartKey, can we update over there also with a similar change.

L534:

            if (range != null) {
              RangeHeader rangeHeader =
                  RangeHeaderParserUtil.parseRangeHeader(range, 0);
              IOUtils.copyLarge(sourceObject, ozoneOutputStream,
                  rangeHeader.getStartOffset(),
                  rangeHeader.getEndOffset() - rangeHeader.getStartOffset());

            }

And also can we add test for S3WrapperInputStream which tests a new method with multiple seek values.
@bharatviswa504 I will handle this. Thanks for your comments.

@runzhiwang
Copy link
Contributor Author

@bharatviswa504 Hi, I move copyLarge into KeyInputStream, and add integration test in TestKeyInputStream for it. And also change all the use of IOUtils.copyLarge.

Copy link
Contributor Author

@runzhiwang runzhiwang left a comment

Choose a reason for hiding this comment

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

I delete the following code because the InputStream in unit test is ByteArrayInputStream, it cannot be cast into KeyInputStream when new S3WrapperInputStream(sourceObject.getInputStream())).

image

It's ok to delete it, because there are other test to cover the case, such as MultipartUpload.robot as the image shows.

image

@adoroszlai adoroszlai requested a review from elek April 22, 2020 15:26
@bharatviswa504
Copy link
Contributor

+1 LGTM.
Thank you @runzhiwang for the improvement.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants