Skip to content
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

LibCore: Port MappedFile to Windows #2353

Merged
merged 3 commits into from
Nov 26, 2024
Merged

Conversation

stasoid
Copy link
Contributor

@stasoid stasoid commented Nov 15, 2024

No description provided.

@stasoid stasoid force-pushed the mmfile branch 2 times, most recently from 9ed38a9 to c3105a3 Compare November 16, 2024 07:57
@stasoid stasoid marked this pull request as ready for review November 16, 2024 08:02
@stasoid stasoid marked this pull request as draft November 19, 2024 07:22
@stasoid
Copy link
Contributor Author

stasoid commented Nov 19, 2024

Marked this as draft because it looks like we better off using mman vcpkg package after all, as suggested by @zacuke. Core::System::mmap cannot be used in AK/BumpAllocator.h because AK should not depend on LibCore. There is a slight caveat that mmap from mman does not support MAP_PRIVATE, but this is not a problem in our case.

@ADKaster
Copy link
Member

If we can't create MAP_PRIVATE | MAP_ANONYMOUS mappings, then an mman wrapper package doesn't seem particularly useful. Unless I'm missing something, we would want to use VirtualAlloc for BumpAllocator. In the ladybird codebase, BumpAllocator is only used by LibRegex. The only other place we use MAP_ANONYMOUS is in the LibGC heap allocator.

It's definitely true that nobody in Ladybird is using a read-write Core::MappedFile, so a read-only mapping for those use cases is just fine.

@stasoid
Copy link
Contributor Author

stasoid commented Nov 20, 2024

MAP_PRIVATE | MAP_ANONYMOUS is not a problem either because:

    // Anonymous mapping returned by this function is effectively always private because
    // the only way to share it is via the handle, which is immediately closed after MapViewOfFile.
    // Hence:
    // * shared anonymous is not supported
    // * private anonymous does not need to use copy-on-write

(mman mmap closes the map handle too)

@stasoid stasoid marked this pull request as ready for review November 21, 2024 08:31
@gmta gmta merged commit b3464d0 into LadybirdBrowser:master Nov 26, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants