Skip to content

Update the C++ FileSink to fall back to write() if memory mapping isn't supported #748

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

Open
godlygeek opened this issue Apr 11, 2025 · 3 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@godlygeek
Copy link
Contributor

For the sake of performance, memray writes to capture files by mapping the capture file into memory and then writing to it as an in-memory array that is eventually synchronized to the filesystem. (See FileSink in sink.cpp / sink.h)

This is fast, but some (uncommon and non-standard) filesystems don't support it.

We should detect if the operations that we need (posix_fallocate and mmap) aren't supported by the filesystem that the capture file is being written to, and if so log a warning and fall back to a slower but more portable path that simply uses write() to append each new chunk of data to the capture file. In order to test this, we'll also need to add a way to force the slower path to be used even if posix_fallocate and mmap are supported. This should probably just be a MEMRAY_NO_MMAP=1 environment variable, or something like that.

@godlygeek godlygeek added enhancement New feature or request good first issue Good for newcomers labels Apr 11, 2025
@XueSongTap
Copy link

Can I work on this ?

@godlygeek
Copy link
Contributor Author

Sure!

@XueSongTap
Copy link

FileSink Fallback Implementation Proposal

Approach

I propose adding a fallback mechanism when memory mapping isn't supported:

  1. Add detection logic to check if posix_fallocate and mmap are supported
  2. Implement environment variable (MEMRAY_NO_MMAP=1) to force fallback path
  3. Create dual write paths - keep existing mmap path but add write() alternative

Key Changes

class FileSink {
private:
    bool d_useMmap{true}; // Flag to determine which path to use
    // ...
};

The constructor would check both environment variable and filesystem support:

d_useMmap = !getenv_bool("MEMRAY_NO_MMAP") && testMmapSupport();

The writeAll method would branch based on this flag:

bool FileSink::writeAll(const char* data, size_t length)
{
    return d_useMmap ? writeWithMmap(data, length) : writeWithWrite(data, length);
}

Questions

I'm unsure about naming the fallback write method. Options:

  • writeWithWrite (too repetitive?)
  • writeFallback
  • writeTraditional

@godlygeek Do you have any preference on the naming convention?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants