You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by "fabriziofortino (via GitHub)" <gi...@apache.org> on 2023/04/10 20:43:08 UTC
[GitHub] [jackrabbit-oak] fabriziofortino opened a new pull request, #893: OAK-10182: use streams to avoid buffer positioning issues leading to corrupted files
fabriziofortino opened a new pull request, #893:
URL: https://github.com/apache/jackrabbit-oak/pull/893
#886 introduced a regression when checksum validation is enabled. The buffer mark is wrongly positioned when the destination file gets written leading to corrupted files. This also explains why the performance results with and without checksum validation were similar.
This PR uses streams and byte arrays instead of channels and ByteBuffers to perform write file operations and checksum. When the latter is enabled, the performance degradation is around 15%.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] nfsantos commented on a diff in pull request #893: OAK-10182: use streams to avoid buffer positioning issues leading to corrupted files
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #893:
URL: https://github.com/apache/jackrabbit-oak/pull/893#discussion_r1162467684
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/Downloader.java:
##########
@@ -175,19 +172,18 @@ public ItemResponse call() throws Exception {
Path destinationPath = Paths.get(item.destination);
Files.createDirectories(destinationPath.getParent());
+
long size = 0;
- try (ReadableByteChannel byteChannel = Channels.newChannel(sourceUrl.getInputStream());
- FileChannel outputChannel = FileChannel.open(destinationPath, StandardOpenOption.CREATE, StandardOpenOption.WRITE)) {
- ByteBuffer buffer = ByteBuffer.allocate(bufferSize);
+ try (InputStream inputStream = sourceUrl.getInputStream();
+ FileOutputStream outputStream = new FileOutputStream(destinationPath.toFile())) {
Review Comment:
I wonder if using buffered output/input streams here would help in speeding up the download? Probably not. What was the rationale for not using buffered streams?
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/Downloader.java:
##########
@@ -175,19 +172,18 @@ public ItemResponse call() throws Exception {
Path destinationPath = Paths.get(item.destination);
Files.createDirectories(destinationPath.getParent());
+
long size = 0;
- try (ReadableByteChannel byteChannel = Channels.newChannel(sourceUrl.getInputStream());
- FileChannel outputChannel = FileChannel.open(destinationPath, StandardOpenOption.CREATE, StandardOpenOption.WRITE)) {
- ByteBuffer buffer = ByteBuffer.allocate(bufferSize);
+ try (InputStream inputStream = sourceUrl.getInputStream();
+ FileOutputStream outputStream = new FileOutputStream(destinationPath.toFile())) {
+ byte[] buffer = new byte[bufferSize];
int bytesRead;
- while ((bytesRead = byteChannel.read(buffer)) != -1) {
- buffer.flip();
+ while ((bytesRead = inputStream.read(buffer)) != -1) {
if (md != null) {
- md.update(buffer);
+ md.update(buffer, 0, bytesRead);
Review Comment:
The checksum could in principle be handed over to another thread, but we have to be careful about managing the buffer. One idea is to have 2 buffers and swap them between the download thread and the checksum thread, so that when one buffer is being used for checksum calculations the other one is used for downloading. Just an idea for future improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] fabriziofortino commented on a diff in pull request #893: OAK-10182: use streams to avoid buffer positioning issues leading to corrupted files
Posted by "fabriziofortino (via GitHub)" <gi...@apache.org>.
fabriziofortino commented on code in PR #893:
URL: https://github.com/apache/jackrabbit-oak/pull/893#discussion_r1162515105
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/Downloader.java:
##########
@@ -175,19 +172,18 @@ public ItemResponse call() throws Exception {
Path destinationPath = Paths.get(item.destination);
Files.createDirectories(destinationPath.getParent());
+
long size = 0;
- try (ReadableByteChannel byteChannel = Channels.newChannel(sourceUrl.getInputStream());
- FileChannel outputChannel = FileChannel.open(destinationPath, StandardOpenOption.CREATE, StandardOpenOption.WRITE)) {
- ByteBuffer buffer = ByteBuffer.allocate(bufferSize);
+ try (InputStream inputStream = sourceUrl.getInputStream();
+ FileOutputStream outputStream = new FileOutputStream(destinationPath.toFile())) {
Review Comment:
I have not tested it. But I have compared this solution with the previous one based on channels and there is no performance degradation when checksum is not enabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] fabriziofortino commented on a diff in pull request #893: OAK-10182: use streams to avoid buffer positioning issues leading to corrupted files
Posted by "fabriziofortino (via GitHub)" <gi...@apache.org>.
fabriziofortino commented on code in PR #893:
URL: https://github.com/apache/jackrabbit-oak/pull/893#discussion_r1162529785
##########
oak-run/src/main/java/org/apache/jackrabbit/oak/run/Downloader.java:
##########
@@ -175,19 +172,18 @@ public ItemResponse call() throws Exception {
Path destinationPath = Paths.get(item.destination);
Files.createDirectories(destinationPath.getParent());
+
long size = 0;
- try (ReadableByteChannel byteChannel = Channels.newChannel(sourceUrl.getInputStream());
- FileChannel outputChannel = FileChannel.open(destinationPath, StandardOpenOption.CREATE, StandardOpenOption.WRITE)) {
- ByteBuffer buffer = ByteBuffer.allocate(bufferSize);
+ try (InputStream inputStream = sourceUrl.getInputStream();
+ FileOutputStream outputStream = new FileOutputStream(destinationPath.toFile())) {
+ byte[] buffer = new byte[bufferSize];
int bytesRead;
- while ((bytesRead = byteChannel.read(buffer)) != -1) {
- buffer.flip();
+ while ((bytesRead = inputStream.read(buffer)) != -1) {
if (md != null) {
- md.update(buffer);
+ md.update(buffer, 0, bytesRead);
Review Comment:
I agree but this would complicate the logic. An even better option could be to use multiple blocking queues where we put the buffer. We can then have separate downstream consumer threads (eg: one for writing the file, and another to compute the checksum). In this way, we can actually decouple the reads from the writes further boosting performance. Complexity will obviously increase. A reactive library might help here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-oak] fabriziofortino merged pull request #893: OAK-10182: use streams to avoid buffer positioning issues leading to corrupted files
Posted by "fabriziofortino (via GitHub)" <gi...@apache.org>.
fabriziofortino merged PR #893:
URL: https://github.com/apache/jackrabbit-oak/pull/893
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org