Use memory-mapped files for updating and for library writing #12560
Conversation
|
Thanks for your pull request, @andralex! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#12560" |
andralex
force-pushed
the
memory-mapped
branch
3 times, most recently
from
734484f
to
c778271
Compare
|
Needs more documentation, clearer structure, and a rationale in the commit message. |
|
Of course OSX has yet another different but not distinct mmfile API... |
| The `Datum` type encodes the mapping mode: Use `ubyte` for read/write mapping | ||
| and `const ubyte` for read-only mapping. | ||
| */ | ||
| struct FileMapping(Datum) |
There was a problem hiding this comment.
The ubyte -> RW / const ubyte -> R mapping is nice, however, someone that sees FileMapping!(ubyte)("bla.txt") might not understand it right away. FileMapping!(Read)/FileMaping(ReadWrite) reads better in my opinion. Also, what happened to plain Write ?
There was a problem hiding this comment.
The idea is to allow support for other units such as uint or ulong. Also, most of the code is common to the read and read-write modes so a template was scoring on both these points.
There's no write-only mapping as far as I understand.
There was a problem hiding this comment.
The idea is to allow support for other units such as uint or ulong.
Martin says don't build in features 'till ya need them. You're going to be sorry you gave me that book :-)
It makes the interface "heavier", and the unused features likely don't even work because they are never tested. It will be trivial to add this if/when it is needed.
There was a problem hiding this comment.
Martin says don't build in features 'till ya need them.
Actually that's exactly what the code is doing: it rejects data types that are not currently used. The template is necessary because there are two modes that have 90% identical code and 99% identical interface.
It makes the interface "heavier"
In fact the opposite is true. The interface is virtually identical across the read-only and read-write versions (only the resize function is not offered in read-only mode). Semantics are identical, too. So the reader only needs to read and absorb ONE interface to understand TWO ways of using memory-mapped files, as opposed to looking at TWO distinct interfaces (subtle duplication for the lose!) that it just so happens are 99% identical with no indication of that in the source code.
Template is the way to go here, no two ways about it. I could use a bool or enum to say "read-only vs read-write" but that is the same aggravation for less upside.
andralex
force-pushed
the
memory-mapped
branch
2 times, most recently
from
b0e56be
to
d8d2339
Compare
dlang-bot
added
Review:Needs Rebase
Review:Needs Work
and removed
Review:Needs Rebase
Review:Needs Work
labels
|
(this is going to be much easier to review if you do a clean rebase. Multiple commits is fine (and encouraged!) but not with a bunch of rebase commits in the middle of it) |
|
@thewilsonator yah, no worries. I'm coding on a laptop in an airport and snuck in a bunch of unrelated commits. Will do a grand |
|
Was wondering why the test failures - it turns out there is a shadow C++ Need a robust solution here, any ideas? First thought that comes to mind is to use a pointer instead of a direct member for the file mapping, which would always be null on the C++ side. |
|
@WalterBright thanks very much for the thorough review! Commit follows. |
andralex
force-pushed
the
memory-mapped
branch
5 times, most recently
from
a992ef8
to
284bdf0
Compare
|
@thewilsonator done squashing |
andralex
force-pushed
the
memory-mapped
branch
7 times, most recently
from
6bc1ae5
to
1ac70ac
Compare
|
What's the matter with buildkite all of a sudden? |
thewilsonator
dismissed
WalterBright’s stale review
All requested changes addressed
thewilsonator
force-pushed
the
memory-mapped
branch
from
0e6a829
to
93ef9e5
Compare
|
(rebased to restart CI with fixed dlang-bot) |
|
Merging. @andralex please consider adding a changelog entry for this. |
This is larger than usual. Functionality added:
OutBuffer. That way, its clients may transparently write straight to files.Even modest libs can get to hundreds of megs. One particular library build at Symmetry produced a library that's 1.7 GB, meaning at a point 3.4 GB of RAM had to be allocated just to write the library out. This saves that much memory.
Hopefully more uses of memory mapping are possible.