#5612 Reduce delays on updating file access#5615
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce main-thread stalls by offloading cache file “last access” timestamp updates to a background worker queue.
Changes:
- Made
LLFileSystem::updateFileAccessTime()const. - Updated
updateFileAccessTime()to post the filesystem timestamp work to the"General"LL::WorkQueueinstead of performing it synchronously on the calling thread.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
indra/llfilesystem/llfilesystem.h |
Marks updateFileAccessTime() as const to reflect non-mutating behavior on the LLFileSystem instance. |
indra/llfilesystem/llfilesystem.cpp |
Moves last_write_time read/update to the "General" work queue to avoid blocking the main thread. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LL::WorkQueue::ptr_t workqueue = LL::WorkQueue::getInstance("General"); | ||
| if (!workqueue) | ||
| { | ||
| LL_WARNS() << "Failed to read last write time for cache file " << file_path << ": " << ec.message() << LL_ENDL; | ||
| return; | ||
| } |
There was a problem hiding this comment.
If the "General" work queue doesn't exist yet, this returns silently and the access timestamp never gets updated. That changes behavior vs the previous synchronous implementation and can affect cache purging. Consider logging a warning and/or falling back to the synchronous timestamp update when the queue is unavailable (or using an existing filesystem thread facility).
There was a problem hiding this comment.
Asset cache is normally relevant only once we are logged in, or at least are logging in. We do not update files during cleanup. But better safe than sorry, will do.
| // General queue maintains multiple threads, so there is a chance of a race | ||
| // condition, when multiple threads are trying to update the same file at | ||
| // the same time. This is not a problem because the last write time is | ||
| // only used to determine which files are old and some missed or | ||
| // extra updates will not cause any significant issues. | ||
| // last_write_time operation is supposed to be atomic, so multiple | ||
| // writes should not result in complications. | ||
| workqueue->post([file_path]() | ||
| { |
There was a problem hiding this comment.
Using WorkQueue::post() can block the calling thread when the queue is at capacity (it waits until space is available). Since this method is called from the main thread specifically to avoid stalls, consider using tryPost() (and intentionally drop the update when full) or otherwise ensure the call cannot block; also check the returned bool so failures don’t silently skip updates.
| constexpr std::time_t time_threshold = 1 * 60 * 60; | ||
|
|
||
| // current time | ||
| const std::time_t cur_time = std::time(nullptr); | ||
|
|
There was a problem hiding this comment.
cur_time is now computed inside the worker lambda, which means backlog/delay in the work queue increases (cur_time - last_write_time) and can cause extra timestamp writes that wouldn’t have happened at call time (defeating the 1h threshold’s purpose). To preserve prior semantics and minimize writes, compute cur_time before posting and capture it into the lambda.
Don't hold main thread for a simple timestamp update, schedule on a worker.