Async reading in FileSystemDataVault #8126
Conversation
frcroth
changed the title
|
I did some testing here with thread count set to 1 and a large file. I requested this route several times in parallel. This way I could confirm that the thread is indeed not blocked in this async variant, and can reply to each request after the respective reading operation is complete. On master, it can only reply after all previous reading operations are complete (meaning the thread is blocked). However, I also noticed that reading the file was consistently faster on master. On average I measured 634 ms on master versus 1160 ms here. So it could be argued that using the synchronous variant (with more threads that consume some memory) is worth the faster loading, compared to this change (that could work with fewer threads). Do you have any opinions on this @frcroth @normanrz ? Of course the testing conditions were pretty specific (ssd on my laptop, warm OS cache, always the same file) and differ somewhat from realistic datastore load conditions. |
|
Async usually helps with throughput when dealing with a lot of requests. It doesn't necessarily yield improvements with just a few requests. However, it shouldn't be much slower, either. So, these results are a bit surprising. |
WalkthroughThe pull request introduces significant enhancements to the WEBKNOSSOS platform, including new features for metadata handling, improved search functionality, and asynchronous file reading capabilities in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@frcroth has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/FileSystemDataVault.scala(2 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[duplication] ~32-~32: Possible typo: you repeated a word
Context: ...calableminds/webknossos/pull/8126) ### Fixed - Fixed a bug during dataset upload in case the...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (2)
CHANGELOG.unreleased.md (1)
30-30: LGTM! Clear and concise changelog entry.
The changelog entry accurately describes the implementation of asynchronous file reading in the datastore filesystem, which aligns with the PR objectives.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/FileSystemDataVault.scala (1)
42-69: Re-evaluate the use of asynchronous file reading for local files
While asynchronous I/O can improve throughput when handling many concurrent operations, it may introduce overhead and potentially degrade performance when reading local files, especially if the read operations are infrequent or involve small files.
Consider conducting thorough performance benchmarks to determine whether the asynchronous approach provides tangible benefits in this context. If it does not offer a significant improvement, it might be preferable to retain synchronous reading to avoid added complexity.
...sos-datastore/app/com/scalableminds/webknossos/datastore/datavault/FileSystemDataVault.scala
Outdated
Show resolved
Hide resolved
...sos-datastore/app/com/scalableminds/webknossos/datastore/datavault/FileSystemDataVault.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Alright, I did some more measuring and it looks like my numbers from yesterday were a little unfair, since they used parallel reading in the async case, while on master, everything was sequential (due to single thread + blocking).
In my new measurements from today, both implementations come in at 600–650ms.
So I’d say we should go for the async variant :)
@frcroth how do you feel about the suggestions by our AI reviewer? I guess it might make sense to add some validations for too-large files / ranges. Not sure about the suggested try/catch, but it does seem plausible too. What do you think?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/FileSystemDataVault.scala (2)
45-45: Usevalinstead ofvarfor channelThe channel variable is only assigned once in the try block. Use
valinstead ofvarand move the declaration inside the try block.Apply this diff:
- var channel: AsynchronousFileChannel = null try { - channel = AsynchronousFileChannel.open(path, StandardOpenOption.READ) + val channel = AsynchronousFileChannel.open(path, StandardOpenOption.READ)
63-67: Improve error handling in CompletionHandlerThe failed method should handle specific exceptions differently. For example, IOException might indicate a temporary failure that could be retried.
Apply this diff:
override def failed(exc: Throwable, buffer: ByteBuffer): Unit = { - promise.failure(exc) + exc match { + case e: java.io.IOException => + logger.warn(s"IO error reading file: ${path}", e) + promise.failure(e) + case e: java.nio.file.NoSuchFileException => + logger.error(s"File not found: ${path}", e) + promise.failure(e) + case e => + logger.error(s"Unexpected error reading file: ${path}", e) + promise.failure(e) + } channel.close() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/FileSystemDataVault.scala(2 hunks)
🔇 Additional comments (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/FileSystemDataVault.scala (1)
42-76: Verify performance impact of async implementation
Based on the PR comments, the async implementation shows slower performance in single-threaded scenarios. This needs investigation:
- The direct ByteBuffer allocation could be a bottleneck
- The Promise creation adds overhead
- Thread pool configuration might need tuning
Let's analyze the codebase for similar patterns:
| val buffer = ByteBuffer.allocateDirect(length) | ||
| var channel: AsynchronousFileChannel = null |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reusing DirectByteBuffer for performance
Creating a direct ByteBuffer for each read operation is expensive. Consider implementing a buffer pool or using a ThreadLocal buffer for better performance.
Would you like me to provide an implementation of a buffer pool?
| val fileSize = Files.size(localPath) | ||
| readAsync(localPath, fileSize - length, length) |
There was a problem hiding this comment.
Add bounds checking for SuffixLength case
The calculation fileSize - length could result in a negative position if length is greater than fileSize. Add validation to prevent this scenario.
Apply this diff:
case SuffixLength(length) =>
val fileSize = Files.size(localPath)
+ val adjustedLength = Math.min(length, Math.toIntExact(fileSize))
+ val position = Math.max(0, fileSize - adjustedLength)
- readAsync(localPath, fileSize - length, length)
+ readAsync(localPath, position, adjustedLength)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val fileSize = Files.size(localPath) | |
| readAsync(localPath, fileSize - length, length) | |
| val fileSize = Files.size(localPath) | |
| val adjustedLength = Math.min(length, Math.toIntExact(fileSize)) | |
| val position = Math.max(0, fileSize - adjustedLength) | |
| readAsync(localPath, position, adjustedLength) |
Steps to test:
Issues:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Improvements