CSharp bindings: add support for file/buffer sizes larger than 2GB#14380
CSharp bindings: add support for file/buffer sizes larger than 2GB#14380Mbucari wants to merge 4 commits intoOSGeo:masterfrom
Conversation
| private class OwnedMemoryFileHandle : Microsoft.Win32.SafeHandles.SafeHandleZeroOrMinusOneIsInvalid { | ||
| private readonly GCHandle dataHandle; | ||
| public string FileName { get; } | ||
| public OwnedMemoryFileHandle(string path, byte[] buffer, long offset, long count) | ||
| : base(ownsHandle: true) { | ||
| FileName = path; | ||
| dataHandle = GCHandle.Alloc(buffer, GCHandleType.Pinned); | ||
| ValidateBufferArgs(buffer, offset, count); | ||
| IntPtr ptr = AddOffset(dataHandle.AddrOfPinnedObject(), offset); | ||
| handle = VSIFileFromMemBuffer(FileName, ptr, (ulong)buffer.LongLength, bTakeOwnership: 0); | ||
| } | ||
| protected override void Dispose(bool disposing){ | ||
| base.Dispose(disposing); | ||
| dataHandle.Free(); | ||
| } | ||
| protected override bool ReleaseHandle() { | ||
| bool success = VSIFCloseL(handle) == 0; | ||
| success &= Unlink(FileName) == 0; | ||
| return success; | ||
| } | ||
| } |
There was a problem hiding this comment.
Pay extra attention to this class. The existing FileFromMemBuffer binding copies the data to Gdal and then closes the VSILFILE.
Here, the VSILFILE is left open. I was torn between returning an open VSILFILE (which is what this does), or closing the VSILFILE and just returning an IDisposable that unpins the memory and unlinks the file.
There was a problem hiding this comment.
I have not had a chance to look in detail yet - but my gut is telling me that if we make a change like that to how the memory is handled we are going to break something downstream. But - I could be misunderstanding what you are saying
There was a problem hiding this comment.
To clarify, this isn't a change to an existing method. This class is used by the entirely new method FileFromMemBufferNoCopy.
Currently, FileFromMemBuffer is only accessible through wrapper_VSIFileFromMemBuffer, which copies the memory to an unmanaged buffer owned by GDAL. This is inefficient, especially for large buffers (#6413 is trying to create a >2GB memory file). This PR exposes VSIFileFromMemBuffer and, FileFromMemBufferNoCopy pins the managed buffer and retains ownership of memory so that it doesn't need to be copied.
There was a problem hiding this comment.
I've thought about this more, and I think FileFromMemBufferNoCopy should behave like wrapper_VSIFileFromMemBuffer by closing the VFFile after it is created. Here's my proposed change:
public sealed class OwnedMemoryFile : IDisposable {
private readonly GCHandle m_dataHandle;
private int m_disposed;
public string Filename { get; }
public bool IsDisposed => m_disposed != 0;
internal OwnedMemoryFile(string filename, GCHandle dataHandle) {
Filename = filename;
m_dataHandle = dataHandle;
}
public void Dispose() {
if (System.Threading.Interlocked.CompareExchange(ref m_disposed, 1, 0) == 0) {
m_dataHandle.Free();
Unlink(Filename);
}
GC.SuppressFinalize(this);
}
~OwnedMemoryFile() => Dispose();
}
public static OwnedMemoryFile FileFromMemBufferNoCopy(string utf8_string, byte[] buffer, long offset, long count) {
ValidateBufferArgs(buffer, offset, count);
GCHandle dataHandle = GCHandle.Alloc(buffer, GCHandleType.Pinned);
IntPtr ptr = AddOffset(dataHandle.AddrOfPinnedObject(), offset);
IntPtr fp = VSIFileFromMemBuffer(utf8_string, ptr, (ulong)buffer.LongLength, bTakeOwnership: 0);
if (fp == IntPtr.Zero) {
dataHandle.Free();
return null;
} else {
VSIFCloseL(fp);
return new OwnedMemoryFile(utf8_string, dataHandle);
}
}There was a problem hiding this comment.
I committed the above changes.
|
@Mbucari Can you rebase against the new Cmake scripts in master |
Mbucari
force-pushed
the
csharp_64bit_buffers
branch
from
c8873c3 to
2d56662
Compare
Done. @rouault can you please re-run the tests? |
|
@runette Please approve if OK for you |
|
@Mbucari - the two PRs seem very different in intent I would be concerned about traceability and auditability if we do that. You could use reflog to revert to before the rebase and start again? |
Mbucari
force-pushed
the
csharp_64bit_buffers
branch
from
2d56662 to
22be4e9
Compare
- Adjust FileFromMemBuffer and VSIF* function definitions to work with buffers and files larger than 2GB. - Add FileFromMemBufferNoCopy method to create an in-memory file from a managed buffer. Returns a SafeHandle to the memory file, which is released on disposal. - Add helper overloads to VSIF* funcitons - Use Int64 for GetCacheMax(), GetCacheUsed(), and SetCacheMax() - Add tests
Make FileFromMemBufferNoCopy behave like the existing FileFromMemBuffer method by closing the handle to the VSI File before returning.
Mbucari
force-pushed
the
csharp_64bit_buffers
branch
from
22be4e9 to
b047276
Compare
|
@runette Success. thanks. |
Mbucari
force-pushed
the
csharp_64bit_buffers
branch
from
9c26ae5 to
2f6b47a
Compare
3 participants
What does this PR do?
@runette
What are related issues/pull requests?
Fixes #6413
AI tool usage
Tasklist
Environment
Provide environment details, if relevant: