8
\$\begingroup\$

I am working on a game engine that should read (among others) compressed files and decompress them into memory. Where the compressed file should be memory mapped.

For that I implemented a simple memory mapped file with a minimal API

In my code, no exceptions are thrown. For hard, unrecoverable errors, i simply call std::terminate, otherwise I make use of raid::result<T>. raid::result<T> is a simple std::expected<T, raid::raid_error_code> where raid::raid_error_code is just an enum class for every possible error in my engine.

//specific/mapped_file.h
#ifndef GAME_SRC_SPECIFIC_MAPPED_FILE_H
#define GAME_SRC_SPECIFIC_MAPPED_FILE_H
#include "common/error.h"
#include <filesystem>
#include <cstddef>
#include "common/unowned_ptr.h"
namespace raid {
    struct mapped_file;
    struct mapped_file_deleter {
        void operator()(mapped_file*);
    };
    using mapped_file_ptr = std::unique_ptr<mapped_file,mapped_file_deleter>;
    result<mapped_file_ptr> open_memfile(const std::filesystem::path& p);
    std::span<const std::byte> get_data(unowned_ptr<const mapped_file> file);
}
#endif

I make use of an opaque data type here because the engine is supposed to be run on linux and windows. An implementation for linux is as follows:

//specific/linux/mapped_file.cpp
#include "specific/mapped_file.h"
#include "common/error.h"
#include "common/log.h"
#include "common/unowned_ptr.h"
#include <cstddef>
#include <filesystem>
#include <span>
#include <sys/mman.h>
#include <system_error>
#include <unistd.h>
#include <fcntl.h>
namespace raid {
    struct mapped_file {
        int fd;
        std::span<std::byte> data;
    };

    result<mapped_file_ptr> open_memfile(const std::filesystem::path& p) {
        std::error_code ec;
        if(!std::filesystem::exists(p)) {
            log_error("File does not exist");
            return make_error(raid_error_code::file_not_found);
        }
        auto file_size = std::filesystem::file_size(p, ec);
        if(ec) {
            log_error("Could not open file for memory mapping: ", ec.message());
            return make_error(raid_error_code::file_access_error);
        }
        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
        auto fd = open(p.c_str(), O_RDONLY);
        if(fd == -1) {
            log_error("Could not open file", p.c_str());
            return make_error(raid_error_code::file_descriptor_failed);
        }
        log_debug("Opened file", p.c_str(), "FD: ", fd);
        auto ptr = mmap(nullptr, file_size, PROT_READ, MAP_PRIVATE, fd, 0);
        if(ptr == MAP_FAILED) {
            log_error("Could not memory map file ", p.c_str());
            close(fd);
            return make_error(raid_error_code::file_memory_map_failed);
        }
        log_debug("Memory mapped File", p.c_str());
        return mapped_file_ptr(new mapped_file{
            .fd = fd,
            .data = std::span{(std::byte*)ptr,file_size},
        });
    }

    void mapped_file_deleter::operator()(mapped_file* f) {
        munmap(f->data.data(), f->data.size_bytes());
        log_debug("munmap", f->fd);
        close(f->fd);
        log_debug("closed FD:", f->fd);
        delete f;
    }

    std::span<const std::byte> get_data(unowned_ptr<const mapped_file> file) {
        return file->data;
    }
}

I am aware of the extra indirection due to it being an opaque data type.

Example usage, using flatbuffers:

auto gameflow_file = open_memfile("gameflow.bin");
if(!gameflow_file) {
    return make_error(gameflow_file.error());
}
auto gameflow_data = get_data((*gameflow_file).get());
auto* gf = serialization::Getgameflow_data(gameflow_data.data());
\$\endgroup\$
2
  • \$\begingroup\$ I assume you have good reason not to simply use boost::interprocess memory mapping? That's cross-platform, FTW. \$\endgroup\$ Commented Sep 16 at 10:24
  • 2
    \$\begingroup\$ @TobySpeight You usually do not use a excavator when a simple shovel will suffice, especially when the excavator would add 4-5 seconds to the compilation process wherever it is needed \$\endgroup\$ Commented Sep 16 at 11:19

4 Answers 4

7
\$\begingroup\$

I don't see any reason to keep the file descriptor open after we have mapped the contents. If we close it immediately after mmap(), we don't need a member for it, so we save a wee bit of storage, too.

I think we wouldn't even need the mapped_file struct, as we'd be able to directly use std::unique-pointer<std::span>.

\$\endgroup\$
2
  • \$\begingroup\$ I did not know you may close the FD after the mapping succeeded. Thanks for the hint. unique_ptr<span> is extremely weird, since a span is unowned memory, so you have "a owning pointer to an unowning data type". \$\endgroup\$ Commented Sep 16 at 11:00
  • 4
    \$\begingroup\$ The Linux man page specifically mentions "After the mmap() call has returned, the file descriptor, fd, can be closed immediately without invalidating the mapping" but it's not quite so clear in the POSIX specification. You're right that the combination of owning and non-owning looks quite strange. Perhaps think of it as the unique-pointer providing the necessary ownership. \$\endgroup\$ Commented Sep 16 at 11:34
6
\$\begingroup\$

Looks like you are using std::unique_ptr to do automatic cleanup. This pattern works very well when we already have an object that we cannot modify by we still need to make sure that we dont forget to clean it up. A classic example for this is the file descriptor. e.g.

auto file = std::unique_ptr<FILE, int(*)(FILE*)>(fopen("file.txt", "rt"), &fclose);

Notice, in this case, we already have a handle to our object, we also have a very simple cleanup function that is already written. We are merely composing preexisting functionality to get something new.

But, in this case, we are doing something a little more complicated. We are trying to model an new abstraction i.e. a region of memory that is mapped to a file. So, it would be cleaner to make a new class and use the good-old destructor to do the clean-up.

We can address the issue of non-throwing construction by making the constructor private and adding a static factory function open to the class.

Here is an example

class MemoryMappedFile {
public:

    static Result<MemoryMappedFile> open(std::filesystem::path & path) noexcept {
        // open_memfile() logic goes here
    };

    MemoryMappedFile(MemoryMappedFile const & ) = delete;
    MemoryMappedFile& operator=(MemoryMappedFile const & ) = delete;

    MemoryMappedFile(MemoryMappedFile&& other) noexcept
    : m_fd{}, m_memory{} {
        swap(other);
    }

    MemoryMappedFile& operator=(MemoryMappedFile&& other) noexcept {
        swap(other);
        return *this;
    }

    std::span<std::byte> data() noexcept { return m_memory;}
    std::span<const std::byte> data() const noexcept { return m_memory; }

    ~MemoryMappedFile() {

        if(m_memory.data()) {
            munmap(m_memory.data(), m_memory.size_bytes());
        }

        if(m_fd) {
            fclose(m_fd);
        }
    }

private:

    void swap(MemoryMappedFile& other) noexcept  {
        std::swap(m_fd, other.m_fd);
        std::swap(m_memory, other.m_memory);    
    }

    MemoryMappedFile(int fd, std::span<std::byte> memory) noexcept : m_fd{fd}, m_memory{memory} {}

    FILE* m_fd;
    std::span<std::byte> m_memory;
};
\$\endgroup\$
2
  • \$\begingroup\$ This would work for linux, but windows uses HANDLEs and other types to represent a file mapping which is why I decided to use a opaque struct with different implementations for the different platforms. \$\endgroup\$ Commented Sep 16 at 16:55
  • \$\begingroup\$ Oh. My bad I overlooked that part. NVM we should be using fopen and FILE in that case it is a c standard function, available on both the platforms. For the Linux part we can use fileno() in linux to get an fd for mmap(). Updated my code to reflect this. \$\endgroup\$ Commented Sep 16 at 17:07
4
\$\begingroup\$

There seems to be a lot of “clever” gymnastics here to keep the implementation private (so it can vary between Linux and Windows), but… this is really a solved problem. What really does all the scaffolding you’ve put in place here do that the classic pattern does not?

// In the header:
class mapped_file
{
public:
    static auto open(std::filesystem::path const&) -> std::expected<std::unique_ptr<mapped_file>, ???>;

    virtual ~mapped_file() = default;

    virtual auto data() const -> std::span<std::byte const> = 0;
};

// In the Linux/POSIX implementation file:
class linux_mapped_file : public mapped_file
{
    std::span<std::byte const> _data;

public:
    explicit linux_mapped_file(std::span<std::byte const> d) : _data{d} {}

    ~linux_mapped_file()
    {
        // unmap
    }

    auto data() const -> std::span<std::byte const> override { return _data; }
};

auto mapped_file::open(std::filesystem::path const& path) -> std::expected<std::unique_ptr<mapped_file>, ???>
{
    // basically the same as your current open_memfile() implementation
    //
    // open and map, and on success, create a pointer to a linux_mapped_file
    // with the data, and return that.
}

// In the Windows implementation file:
//
// basically the same, except with Windows stuff.

The usage is basically the same:

if(auto gameflow_file = mapped_file::open("gameflow.bin"); gameflow_file)
    auto* gf = serialization::Getgameflow_data(gameflow_file->data());
else
    return make_error(gameflow_file.error());

You could go even further and avoid the pointer semantics that come with unique_ptr by returning a wrapper class that just holds a pointer the abstract base class, and forwards any calls through it. That is, just use the classic pImpl pattern.

You could even avoid dynamic allocation altogether if you had a buffer in the class large enough to hold the instantiated derived classes in situ. Again, this is not novel. This is a bog-standard pattern.

My point is: what is the sense in re-inventing the wheel here? This is a very old, very solved problem, with traditional, universally-known solutions that work just fine.

That being said, I think you’ve missed a powerful opportunity here to work with the standard library, instead of around it. Or at least to be inspired by it.

Think about what you’re making. It’s a file stream. It just happens to be a file stream whose internal buffer buffers the whole file by means of mapping. But… it’s still a file stream.

If you created a mapped file buffer derived from std::streambuf, then you could throw that in any old std::istream, and be able to read the file data just as if it were any other file. It would be transparent to client code whether the file is mapped or not.

Okay, if you don’t want to use standard IOstreams, I’m the first person who will understand. They suck on so many levels. They are also character-based, and your example code seems to suggest that you want binary I/O, which logically should be byte-based.

However, you can still take inspiration from the IOstreams library. In other words, you should have an abstract interface for input streams, and concrete input streams that read from un-cached files, memory-mapped files, in-memory data buffers, and maybe even network streams and other stuff. And of course, you could even have more specific abstract interfaces if you need them, such as an abstract mapped file interface, with concrete implementation for specific platforms. But you probably don’t need that; a single, universal input stream API could probably serve all purposes, then you just make concrete implementations for any specific type of behaviour and/or platform.

There is a proposal for a next-generation standard I/O library that includes mapped files. It’s kinda up in the air now because we really need a proper asynchronous programming model in C++, and executors—already in C++26—should provide for that. It’s extremely low-level, so it probably isn’t what you have in mind, but it does show the idea of a base file_handle class, and then a mapped_file_handle derived from that.

I recommend also looking into the design of Boost.Iostream. It is a bit old and creaky, but its design is very powerful. It uses generic concepts of sources and sinks, and filters. So, for example, you would make a mapped file source, and then you could make a “decompress” filter, and use them like so:

auto in = boost::iostreams::filtering_istream{decompressor{} | mapped_file_source{"gameflow.bin"}};

deserialize(in);

Now let’s say your game data is not only compressed, but encrypted, to prevent snooping. No problem:

auto in = boost::iostreams::filtering_istream{decrypter{} | decompressor{} | mapped_file_source{"gameflow.bin"}};

deserialize(in);

As you can see, the usage of the stream remains unchanged.

I don’t suggest you use Boost.Iostream. It’s pretty crusty in places. I mean, I would expect a pipeline interface to look more like: mapped_file_source{"gameflow.bin"} | decompress | decrypt. And you could use it like: deserialize(mapped_file_source{"gameflow.bin"} | decompress | decrypt). Also, it’s still character-based, and it is designed to work with the standard IOstreams library, which I would just avoiding.

What I suggest is you look at the design of Boost.Iostream—in particular, the concepts of sinks, sources, and filters, and of creating pipelines of filters—and use that as the basis for a solid I/O API. You could have an abstract “source” base class, and then your mapped file would be derived from that. Or you could use concepts and maybe mostly avoid dynamic polymophism (though you might still want to use it for abstracting away platform-specific details). Just come up with a really good, really powerful, really flexible, generic “source” interface, and then specialize it for mapped files… or anything else. Add the idea of pipe-able input filters, and then make your deserialization and other such functions take generic sources (which may be filtered or not). This can be done without too much complexity, yet still give enormous flexibility and still very good performance.

\$\endgroup\$
6
  • \$\begingroup\$ What does the OP's implementation give them which yours doesn't, you ask? Theirs doesn't have any virtual functions, so no vtable pointer in each object, and no indirection for those method calls. Not a big deal since only the OS-specific syscall stuff like create / unmap (and potentially mremap) needs to be virtual, not access to the buffer. But it still feels silly to have virtual functions which will only ever have one target in a given build of the program. Maybe that's something LTO optimization can see and devirtualize, but still needs the vtable pointer in each object. \$\endgroup\$ Commented Sep 17 at 8:24
  • \$\begingroup\$ The OP's solution has other stuff instead of a vtable pointer; I haven't compared the total sizes. \$\endgroup\$ Commented Sep 17 at 8:26
  • 1
    \$\begingroup\$ I considered using inheritance, but ultimately decided against it because as @PeterCordes mentioned, there is exactly 1 implementation ever existing in a build, which makes inheritance itself not useful here \$\endgroup\$ Commented Sep 17 at 13:37
  • \$\begingroup\$ While it makes sense to take efficiency into consideration, if you are counting cycles in an operation where you are loading a file from mass storage and mapping it into memory… where you are literally talking about the difference between 1 indirect call versus 2 (which may even be optimized away!!!) in an operation involving multiple kernel calls… then you’ve kinda lost the plot. But okay, fine, whatever, even then, my point stands: I also mentioned pImpl. Why not use that? Again: why reinvent the wheel when multiple wheels that would work already exist? \$\endgroup\$ Commented Sep 17 at 19:41
  • \$\begingroup\$ Keep in mind also that “I’ll only ever need the one implementation” is kinda of a silly point in light of the fact that you should have a hierarchy of input stream types, so that you can treat any input source—mapped file, un-mapped file, network stream, in-memory buffer, etc.—generically. Whatever you “gain” from not having a couple vtables will be lost many times over when you have to duplicate entire algorithms for different input types. \$\endgroup\$ Commented Sep 17 at 19:45
2
\$\begingroup\$

The Deleter

First, the logic before delete f; would ideally be part of ~mapped_file(), even if that forces you to make the type slightly less opaque. A mapped_file would then properly unmap and close the memory-mapped file without needing to be owned by a specific wrapper.

If you do not go that route: Looking at the implementation of mapped_file_deleter, it has no data members and one member function that only dereferences its one argument. You could therefore make its operator() a static member function, which can help optimize your std::unique_ptr: it will not then need to be passed a this pointer, and instances will not need unique addresses.

Recommendation

Have one type of object, a mapped_file.class, which manages its resources with RAII.

Replace raid::mapped_file_deleter::operator()(mapped_file*) with ~mapped_file().

Make the current mapped_file struct private: data members, unless you need an extern "C" interface, in which case the C++ class should be a wrapper for the C struct.

The open_memfile and get_data functions should become member functions, with implementations in "windows/mapped_file.cpp" or "linux/mapped_file.cpp".

I would advise you to just write separate linux/mapped_file.h and windows/mapped_file.h headers too, adding the appropriate directory to your include path in your build system. Another way to do it without bringing in any OS-specific headers or any of the overhead of virtual functions is:

#include <cstddef>
#include <span>

namespace raid {

class mapped_file {
    struct mapped_file_internals; // Incomplete type.

    mapped_file_internals* internals = nullptr;

    mapped_file_internals* open_memfile(const char* filename);

public:
    mapped_file() = default;
    mapped_file(mapped_file&&) = default;
    mapped_file& operator=(mapped_file&&) = default;
    // Move-only, as only one mapping of a given file should exist at a time:
    mapped_file(const mapped_file&) = delete;
    mapped_file& operator=(const mapped_file&) = delete;

    mapped_file(const char* filename)
    : internals(open_memfile(filename))
    {}

    ~mapped_file() noexcept;

    explicit constexpr operator bool() const noexcept
    {
        return internals != nullptr;
    }

    std::span<std::byte> get() const; // Must not outlive the parent object.
    // Which attributes this can have depends on implementation details.

    void open(const char* filename); // Or return raid::raid_error_code

    // Add other stuff here.
};

} // end namespace raid

The Linux Implementation

As mmap() increments the reference count of the file descriptor when any mappings to it are added and decrements when all mappings are removed, you in fact want to open the file, map with MAP_PRIVATE, then close the file descriptor. There is no need to store it; it will be unmapped after the destructor calls munmap(). This might allow you to store only the range of memory returned by mapped_file::get().

Other Considerations

If these mapped files are, in practice, only used as inputs to the compressor, they might not need to be a full-fledged class of their own. You could then have swap and release operations, so you can pass a mapped_file&& to the function that consumes the file data and have it ensure the memory is not unmapped during the lifetime of any references that would be left dangling.

\$\endgroup\$
3
  • \$\begingroup\$ Can you explain the "leaking resources" in detail? open_memfile is the only function that can create a mapped_file \$\endgroup\$ Commented Sep 17 at 5:51
  • \$\begingroup\$ But mapped_file is an opaque struct. You can't allocate it outside of mapped_file.cpp \$\endgroup\$ Commented Sep 17 at 6:21
  • 1
    \$\begingroup\$ @Raildex Thank you for the correction. I’ve deleted the inaccurate comment and will take another look at my answer when I’m less tired. \$\endgroup\$ Commented Sep 17 at 6:23

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.