-
Notifications
You must be signed in to change notification settings - Fork 81
Add support for Memory Map and 64 bit files in LLAPRFile class #4567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Added support for 64 Bit file to llapr.cpp - open64 - seek64 - read64 - write64 - static size64 - static seek64 - static readEx64 - static writeEx64 Added support for Memory map files - LLARPFile with mmap Read/Write flags - openMemoryMap - openMemoryMap64 - openMemoryMap - no size specified - memoryMapAssign - memoryMapAssign64 Updated #define's for LL_APR_R etc. to use newer ARP_FOPEN_READ as the old ARP_READ is deprecated Updated llassert_always to use signed 64bit limit instead of signed 32bit
All contributors have signed the CLA ✍️ ✅ |
recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution!
I did a quick check, the issues I noticed are mentioned in the comments.
indra/llcommon/llapr.cpp
Outdated
// If need to zero out the file, then use memset over the memory map | ||
if (zero_out) | ||
{ | ||
memset(mMMapFile->mm, 0, sizeof(file_size)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably should be just file_size
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the good catch. Will be fixed.
indra/llcommon/llapr.cpp
Outdated
@@ -368,7 +401,7 @@ apr_status_t LLAPRFile::open(const std::string& filename, apr_int32_t flags, LLV | |||
apr_off_t offset = 0; | |||
if (apr_file_seek(mFile, APR_END, &offset) == APR_SUCCESS) | |||
{ | |||
llassert_always(offset <= 0x7fffffff); | |||
llassert_always(offset <= 0x7fffffffffffffff); //Use 64 bit assert instead of 32 bit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's needed for 32-bit path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internally, the APR library uses a 64 bit pointer to track the offsets and file size. But to keep consistent with existing code, I will change back.
{ | ||
llassert_always(offset <= 0x7fffffffffffffff); | ||
apr_size_t one_char = 1; | ||
apr_file_write(mFile, "\0", &one_char); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this increase the file's size for 1 byte?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it so that it subtracts 1 byte from the init_file_size so that the user of the API does not need to do it in the call. I was subtracting 1 from my calculated file size in my original test, but this makes more sense and is cleaner for the user of the API.
On windows, you need to write the last byte of the file otherwise windows will not allocate the full file. And when you try to use the memory map on a file that is smaller then the memory map, it will give you errors. The file the memory map works with has to be a fixed size.
If you want to increase the file size, you have to remove the memory map, modify the file and open the memory map again.
indra/llcommon/llapr.h
Outdated
// 64 bit Memory map version of open file (File must be fixed size) | ||
apr_status_t openMemoryMap64(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, LLVolatileAPRPool* pool = NULL, S64* sizep = NULL); | ||
// 32 bit Memory map version of open file and initalizes with size and can zero out (File must be fixed size) | ||
apr_status_t openMemoryMap(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, S32 init_file_size, bool zero_out, LLVolatileAPRPool* pool = NULL, S32* sizep = NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing spaces here make pre-commit fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah the bane of my existence. Visual Studio 2022 seems to really like moving tabs up a line when you press delete on the line above or backspace a line to remove it. It has been fixed.
@minerjr, thanks for the PR. You mention an intention to implement a faster texture caching system. Would you mind creating an issue or discussion first to share your vision? We have some upcoming work that might complement your ideas, and I'd love to ensure we can coordinate effectively. A quick design discussion beforehand often leads to smoother implementations and better integration. |
Added new 32 bit method for creating a 32 bit memory map from the constructor. Reverted 32 bit methods to use a 32 bit 2GB assert to be consistent. Modified openMemoryMap and openMemoryMap64: - Subtracted 1 from init_file_size, to prevent the need for users to specify -1 in the size passed in. - Fixed memset to use file_size instead of sizeof(file_size). - Fixed file_size to use init_file_size instead of offset as offset is 1 byte short Removed end of line white space in header file.
I have read the CLA Document and I hereby sign the CLA |
Hi thanks for the feedback. I will make the noted changes. @bennettgoble I wrote my thoughts on 2 possible solutions in this feature request form. https://feedback.secondlife.com/feature-requests/p/improved-texture-fast-cache-system Let me know what you think of the ideas as any feedback is welcome. I have the second proposal working mostly implemented as a proof of concept. There are a few things to work out when it comes to replacing the need to load from normal cache when the fast cache can handle it at times. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR extends the LLAPRFile class to support 64-bit file operations and memory-mapped files, removing the 2GB file size limitation and enabling more efficient file access patterns. The changes introduce new 64-bit variants of existing file operations and comprehensive memory mapping functionality.
- Adds 64-bit file operation methods (open64, read64, write64, seek64, etc.) to handle files larger than 2GB
- Introduces memory-mapped file support with various initialization options for zero-copy file access
- Updates deprecated APR flag definitions to use modern APR_FOPEN_* constants
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
File | Description |
---|---|
indra/llcommon/llapr.h | Adds 64-bit method declarations, memory map support, and updates deprecated APR flag definitions |
indra/llcommon/llapr.cpp | Implements 64-bit file operations and memory mapping functionality with proper error handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
indra/llcommon/llapr.cpp
Outdated
@@ -338,6 +378,7 @@ apr_status_t LLAPRFile::close() | |||
mCurrentFilePoolp = NULL ; | |||
} | |||
|
|||
ret = ret | mmap_ret; // Flag normal or memory map return state | |||
return ret ; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using bitwise OR to combine return codes can mask important error information. If both ret
and mmap_ret
are non-zero error codes, the result may not represent either error correctly. Consider using a more explicit error handling approach, such as returning the first error encountered or combining errors in a way that preserves diagnostic information.
return ret ; | |
// If both operations failed, log both errors | |
if (ret != APR_SUCCESS && mmap_ret != APR_SUCCESS) | |
{ | |
LL_WARNS("APR") << "Both apr_file_close and apr_mmap_delete failed: " | |
<< "apr_file_close returned " << (S32)ret << ", " | |
<< "apr_mmap_delete returned " << (S32)mmap_ret << LL_ENDL; | |
// Return the file close error code by convention | |
return ret; | |
} | |
// If only one failed, return the error code | |
if (ret != APR_SUCCESS) | |
{ | |
return ret; | |
} | |
if (mmap_ret != APR_SUCCESS) | |
{ | |
return mmap_ret; | |
} | |
// Both succeeded | |
return APR_SUCCESS; |
Copilot uses AI. Check for mistakes.
indra/llcommon/llapr.cpp
Outdated
s = apr_mmap_create(&mMMapFile, mFile, 0, file_size, mmap_flags, apr_pool); | ||
} | ||
|
||
if (!mFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition checks !mFile
but should also check !mMMapFile
to be consistent with the error handling pattern used in other openMemoryMap methods. If memory map creation fails but file opening succeeds, this will not trigger cleanup properly.
if (!mFile) | |
if (!mFile || !mMMapFile) |
Copilot uses AI. Check for mistakes.
indra/llcommon/llapr.cpp
Outdated
LLAPRFile::LLAPRFile(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, S32 init_file_size, bool zero_out, LLVolatileAPRPool* pool) | ||
: mFile(NULL), | ||
mMMapFile(NULL), | ||
mCurrentFilePoolp(NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistent indentation in the member initializer list. The indentation should match the other members above it for better code consistency.
mCurrentFilePoolp(NULL) | |
mMMapFile(NULL), | |
mCurrentFilePoolp(NULL) |
Copilot uses AI. Check for mistakes.
indra/llcommon/llapr.h
Outdated
#define LL_APR_AB (APR_CREATE|APR_WRITE|APR_BINARY|APR_APPEND) | ||
#define LL_APR_RPB (APR_READ|APR_WRITE|APR_BINARY) // "r+b" | ||
#define LL_APR_WPB (APR_CREATE|APR_TRUNCATE|APR_READ|APR_WRITE|APR_BINARY) // "w+b" | ||
// Updated to match the newer #define as the older API_ defines are depricated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"depricated" should be spelled "deprecated".
// Updated to match the newer #define as the older API_ defines are depricated. | |
// Updated to match the newer #define as the older API_ defines are deprecated. |
Copilot uses AI. Check for mistakes.
indra/llcommon/llapr.cpp
Outdated
} | ||
else | ||
{ | ||
LL_WARNS() << "ARP File does not contain a valid Memory Map File" << LL_ENDL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ARP" should be "APR" to match the library name (Apache Portable Runtime).
Copilot uses AI. Check for mistakes.
indra/llcommon/llapr.h
Outdated
LLAPRFile(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, LLVolatileAPRPool* pool = NULL ); | ||
// Opens a initialized 64 bit memory map file (File must be fixed size) | ||
LLAPRFile(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, S64 init_file_size, bool zero_out, LLVolatileAPRPool* pool = NULL); | ||
// Opens a initialized 32 bit memory map file (File must be fixed size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"initialized" should have an article: "Opens an initialized 32 bit memory map file".
// Opens a initialized 32 bit memory map file (File must be fixed size) | |
// Opens an initialized 32 bit memory map file (File must be fixed size) |
Copilot uses AI. Check for mistakes.
indra/llcommon/llapr.h
Outdated
apr_status_t openMemoryMap64(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, LLVolatileAPRPool* pool = NULL, S64* sizep = NULL); | ||
// 32 bit Memory map version of open file and initalizes with size and can zero out (File must be fixed size) | ||
apr_status_t openMemoryMap(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, S32 init_file_size, bool zero_out, LLVolatileAPRPool* pool = NULL, S32* sizep = NULL); | ||
// 64 bit Memory map version of open file and initalizes with size and can zero out (File must be fixed size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"initalizes" should be spelled "initializes".
// 64 bit Memory map version of open file and initalizes with size and can zero out (File must be fixed size) | |
// 32 bit Memory map version of open file and initializes with size and can zero out (File must be fixed size) | |
apr_status_t openMemoryMap(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, S32 init_file_size, bool zero_out, LLVolatileAPRPool* pool = NULL, S32* sizep = NULL); | |
// 64 bit Memory map version of open file and initializes with size and can zero out (File must be fixed size) |
Copilot uses AI. Check for mistakes.
indra/llcommon/llapr.h
Outdated
apr_status_t openMemoryMap64(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, LLVolatileAPRPool* pool = NULL, S64* sizep = NULL); | ||
// 32 bit Memory map version of open file and initalizes with size and can zero out (File must be fixed size) | ||
apr_status_t openMemoryMap(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, S32 init_file_size, bool zero_out, LLVolatileAPRPool* pool = NULL, S32* sizep = NULL); | ||
// 64 bit Memory map version of open file and initalizes with size and can zero out (File must be fixed size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"initalizes" should be spelled "initializes".
// 64 bit Memory map version of open file and initalizes with size and can zero out (File must be fixed size) | |
// 32 bit Memory map version of open file and initializes with size and can zero out (File must be fixed size) | |
apr_status_t openMemoryMap(const std::string& filename, apr_int32_t flags, apr_int32_t mmap_flags, S32 init_file_size, bool zero_out, LLVolatileAPRPool* pool = NULL, S32* sizep = NULL); | |
// 64 bit Memory map version of open file and initializes with size and can zero out (File must be fixed size) |
Copilot uses AI. Check for mistakes.
indra/llcommon/llapr.h
Outdated
|
||
apr_file_t* getFileHandle() {return mFile;} | ||
|
||
// Assignes a variable to a memory map address (used for setting a variable pointer to the memory map (input needs to be casted) with 32 bit offset | ||
apr_status_t memoryMapAssign(void** addr, S32 offset); | ||
// Assignes a variable to a memory map address (used for setting a variable pointer to the memory map (input needs to be casted) with 64 bit offset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Assignes" should be spelled "Assigns".
// Assignes a variable to a memory map address (used for setting a variable pointer to the memory map (input needs to be casted) with 64 bit offset | |
// Assigns a variable to a memory map address (used for setting a variable pointer to the memory map (input needs to be casted) with 32 bit offset | |
apr_status_t memoryMapAssign(void** addr, S32 offset); | |
// Assigns a variable to a memory map address (used for setting a variable pointer to the memory map (input needs to be casted) with 64 bit offset |
Copilot uses AI. Check for mistakes.
indra/llcommon/llapr.h
Outdated
|
||
apr_file_t* getFileHandle() {return mFile;} | ||
|
||
// Assignes a variable to a memory map address (used for setting a variable pointer to the memory map (input needs to be casted) with 32 bit offset | ||
apr_status_t memoryMapAssign(void** addr, S32 offset); | ||
// Assignes a variable to a memory map address (used for setting a variable pointer to the memory map (input needs to be casted) with 64 bit offset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Assignes" should be spelled "Assigns".
// Assignes a variable to a memory map address (used for setting a variable pointer to the memory map (input needs to be casted) with 64 bit offset | |
// Assigns a variable to a memory map address (used for setting a variable pointer to the memory map (input needs to be casted) with 32 bit offset | |
apr_status_t memoryMapAssign(void** addr, S32 offset); | |
// Assigns a variable to a memory map address (used for setting a variable pointer to the memory map (input needs to be casted) with 64 bit offset |
Copilot uses AI. Check for mistakes.
@minerjr I hope you don't mind --I've requested a copilot code review on these changes. Copilot reviews can generate some useful feedback, but can also be incorrect at times. Feel free to review and resolve/address commentary from the bot. :) |
@bennettgoble Sure sounds good, I will take a look at implementing in the morning. I quickly scanned the report and saw a few that I will put in right away and they make sense. Mostly typo's in the comments and a few extra formatting and error handling things. |
APR file stuff needs APR pools: those are a PITA to manage and have been the cause for many crashes (especially on shutdown) and memory leaks in the viewer code over years. Why not getting fully rid of LLAPRFile and instead implement everything through LLFile (i.e. delegating to the OS the file concurrency access management, something all modern OSes can deal with without problem) ? FYI, this is what I did in the Cool VL Viewer, years ago, and I am so happy I did it (so much stabler and so much easier to maintain) ! What I did is to implement LLFile() constructors and methods able to deal with LLFile instances, in excess of the static methods. I also migrated LLAPRFile::readEx() and LLAPRFile::writeEx() as static methods of LLFile (as for the 2GB limit with them, removing it is just a matter of using S64's for offsets, instead of S32's). Adding memory mapping to LLFile() instances would be possible too... Feel free to borrow the Cool VL Viewer code for all this (you also have permission to re-license it as LGPL/whatever you need) ! You will also find interesting touch(), [un]lock() and createFileSymlink() methods in my improved/expanded code, as well as a workaround for a Wine bug (existing in current Wine 9 release), and since, unlike most TPVs, LL's viewer does not yet have a Linux build, it is relevant to it. PS: oh, and I also got fully rid of LLLFSThread, which was barely used anyway, and can advantageously be replaced with my new LLFile instances and the LL:WorkQueue's for threading file operations. |
In considering the conversation here - I'm generally leaning towards Henri's proposal here. Nowadays async I/O is a well solved problem at the OS level, and if the broader concern is: better I/O then we should likely be looking in the direction of relying more so on modern OS primitives to aid in this rather than expanding LLAPRFile which is unlikely to yield many major improvements to what we already have (and to Henri's point, still requires we manage the APR pool carefully to ensure no leaks and minimal crashes). For concurrency, we're potentially changing that up here in the near future with some better concurrency primitives. I'll be building out a proof of concept for that this week (assuming 2025.06's release doesn't get in the way). So stay tuned on that. For now just tossing it into work queue is probably okay as a short term solution. |
I would not mind at all if you wanted to submit your code to the LL viewer as I also know the pain of the APR library. @bennettgoble Do you want to take @vldevel up on their offer? I could close this PR if you wanted so they could submit their code. I could then work on adding memory maps to added code after or they are may if they wish. I took all the Copilot suggestions and implemented them. The only one I tweaked was the first one to add warnings on the single error events as the combined had a warning. I noticed a couple of the suggestions seem like they would conflict with each other so I put all the changes into a single update. Should I push it up or just close the PR as is? |
Fixed all the issues tracked by Copilot. Fixed up several typos and formatting issues. Fixed up close method to be more verbose in warnings issued on error and better handling of the different return codes. Fixed openMemoryMap(64) methods to check also if the mMMapFile is also valid to give a warning and error out.
@bennettgoble @Geenz I pushed up the copilot changes. I did it as a single update instead of trying to use the suggestions directly as some needed some tweaks. Nice feature though. I should try to use it offline before submitting in the future. I pushed up the changes so that it's at least in the best version of my PR. If you want to close the PR down and use @vldevel solution that is fine as well. |
I can assure you it is working beautifully: I'm using it for the audio decode manager and the texture cache, which replaced the LLLFSThread() two only use cases (and even there, the texture cache case was disabled due to instability (texture corruptions): my implementation of this cache is now 100% threaded using work queues), and I also converted the viewer object cache to threaded I/O file operations, with my LLFile() implementation and LL:WorkQueue's. My viewer has had them for a long time now, and the implementation is rock-solid (0 bug detected so far), under all three OSes...
FYI, before my rewrite of the texture cache with LLFile/LLWorkQueue, I had an IOCP/io_uring implementation/rewrite of LLLFSThread (from Kathrine Jansma) in the Cool VL Viewer (maybe this is what you are considering for your own implementation)... But in the end I found the LLFile/LLWorkQueue solution to be not only easier, but to provide the same performances (*), while being OS-agnostic (i.e. it also works for macOS, which got no IOCP (Windows) or io_uring (Linux) equivalent)... (*) The viewer is mostly limited by the network bandwidth and latency (150ms "ping" or so, overseas), not by modern storage bandwidth: even a modern hard disk can do 100MB/s, which a FTTH link barely manages to provide, and for SSDs (even "slow" SATA ones), the gap with the max network bandwidth and network latency (vs I/O ops per second) is even larger. |
Description
Currently the LLAPRFile class only supports 32 bit files and 2 GB file limits. Also, memory mapped files are not supported.
This PR adds additional native 64 bit methods using new 64 bit named versions of the existing LLAPRFile methods. This will allow files over 2 GB from being accessed and loaded.
This PR also adds new methods for loading a memory map file, creating and zeroing out a memory map file as well as assign object pointers to a memory map region.
With a memory map, you could map a data structure directly to memory, with out have to first load the object to RAM, then create a new object, then copy from the read in memory from file to the new object. This also removes the need for File IO commands for accessing data stored in the file.
This could be helpful for such things as the Texture Fast Cache, or other resources which are cached in a native format that does not require conversion of the data. Could be also used for normal Texture Cache for mapping to a Raw texture to be decoded which would remove the need for File IO to read the data in. If a cache is larger then 2GB for a unified file, 64 file support would allow the file to be used or memory mapped.
Also, the #defines of the LLAPR.h file are using deprecated APR defines. Propose to update to the correct naming from the library which will remove deprecated warnings.
Change summary:
Added support for 64 Bit files to llapr.cpp
Added support for Memory map files
Updated #define's for LL_APR_R etc. to use newer ARP_FOPEN_READ as the old ARP_READ is deprecated
Updated llassert_always to use signed 64bit limit instead of signed 32bit
Related Issues
Issue Link: Address issue: #4566
Additional Notes
This will be a basis for a possible enhanced Texture Fast Cache system.