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

Overhaul API #33

Closed
sfackler opened this Issue May 7, 2017 · 11 comments

Comments

Projects
None yet
6 participants
@sfackler
Copy link
Contributor

sfackler commented May 7, 2017

  • Remove MmapView and MmapViewSync types
    • While it is very common to need something like these types when working with memory maps, the exact semantics tend to differ enough that you can't actually use these types. In addition, they're simple enough that removing them won't have too much of a negative impact.
  • Move the "unsafety location" from access to construction
    • Define two types - Mmap and MmapMut for readable and readable/writable mappings respectively. They implement Deref<Target= [u8]>/DerefMut as appropriate. In contexts where access to the mapped region could cause faults (e.g. file backed maps on Unix) make the constructor unsafe.
  • Keep a couple of convenience constructors on Mmap and MmapMut, but move advanced configuration to an OpenOptions-style builder. The builder will provide a cross-platform API directly with OS specific extension traits.
@brson

This comment has been minimized.

Copy link

brson commented May 9, 2017

This looks like something somebody new to the memmap crate could pick off, but maybe needs some guidance from @danburkert.

@jonas-schievink

This comment has been minimized.

Copy link
Contributor

jonas-schievink commented May 9, 2017

Note that a way of converting Mmap <=> MmapMut is required in order to not lose features like the ability to write a safe JIT. Also I think someone requested a way to make unreadable but executable mappings, how would those be integrated?

@josephDunne josephDunne referenced this issue May 9, 2017

Open

Huge Pages #26

@sfackler

This comment has been minimized.

Copy link
Contributor Author

sfackler commented May 9, 2017

We discussed adding a third MmapUnsafe type for more "exotic" cases that wouldn't implement Deref but would just expose the pointer and len.

@bugaevc bugaevc referenced this issue May 10, 2017

Merged

Overhaul the API #42

@ghost

This comment has been minimized.

Copy link

ghost commented May 11, 2017

Hey hi! II'm new to memmap as @brson said, but I'd like to help here :)

@brson

This comment has been minimized.

Copy link

brson commented May 13, 2017

@z1mvader it looks like @bugaevc has a [PR] opened on this subject already.

@danburkert

This comment has been minimized.

Copy link
Owner

danburkert commented May 22, 2017

@bugaevc landed the first cleanup patch, thanks!

I think the next major piece that needs to be overhauled is the Protection system. Ideally, I think we could get rid of it and replace it with more descriptive methods. For instance:

impl<'a> FileMmapOptions<'a> {
    /// mmap(ptr, len, PROT_READ, MAP_SHARED, fd, offset)
    pub fn map(&self) -> Result<Mmap> { ...  }
    /// mmap(ptr, len, PROT_READ | PROT_EXECUTE, MAP_SHARED, fd, offset)
    pub fn map_execute(&self) -> Result<Mmap> { ... }
    /// mmap(ptr, len, PROT_READ | PROT_WRITE, MAP_SHARED, fd, offset)
    pub fn map_mut(&self) -> Result<MmapMut> { ... }
    /// mmap(ptr, len, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, offset)
    pub fn map_copy(&self) -> Result<MmapMut> { ...  }
}

impl AnonymousMmapOptions {
    /// mmap(ptr, len, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANON, fd, offset)
    pub fn map_mut(&self) -> Result<MmapMut> { ...  }
}

impl Mmap {
    // mprotect(ptr, len, PROT_READ | PROT_WRITE)
    pub fn make_mut(mut self, protection: Protection) -> Result<MmapMut> { ... }
}

impl MmapMut {
    // mprotect(ptr, len, PROT_READ)
    pub fn make_read_only(mut self) -> Result<Mmap> { ...  }
    // mprotect(ptr, len, PROT_READ | PROT_EXEC)
    pub fn make_read_exec(mut self) -> Result<Mmap> { ...  }
    ...
    // flush methods
}

Open questions:

  • Does this provide the full range of flexibility that people need? Obviously not every option provided by the mmap system call is reproduced, but ideally we can cover all common cross-platform usecases with this API.
  • Will this map well to Windows, especially in light of #31
  • Naming
@retep998

This comment has been minimized.

Copy link
Contributor

retep998 commented May 26, 2017

Note that a way of converting Mmap <=> MmapMut is required in order to not lose features like the ability to write a safe JIT. Also I think someone requested a way to make unreadable but executable mappings, how would those be integrated?

What you really want here is an API for managing virtual memory, not an API for anonymous memory mapped files. While they may both use mmap on linux, on Windows they are extremely different.

In my opinion, the needs and use cases that people express for memory maps are probably quite skewed by the fact that people abuse memory maps for virtual memory management due to the a lack of good alternative. As a result, the sooner we get a dedicated API for virtual memory management, the sooner we'd be able to know what we actually need from regular memory maps.

@brson

This comment has been minimized.

Copy link

brson commented Jul 6, 2017

I see all the op checkboxes are checked but that @danburkert has a linked PR open. So I take it that there's one more thing to do here.

Closing this will unblock #32 and #37

@Lapin0t

This comment has been minimized.

Copy link

Lapin0t commented Jul 30, 2017

I would love to have a higher-level API which would much more look like an allocator, ie something that return something more or less like Box<T> to the user. The new placement protocol might be interesting to look at too. I'm currently experimenting with things somewhat like this in a personal project (but in a specific case and without seeking to write generic code), but i would be interested to contribute back to this crate afterwards. Does this sound interesting or is it out of scope for this crate?

@danburkert

This comment has been minimized.

Copy link
Owner

danburkert commented Aug 17, 2017

I think higher level abstractions are going to be out of scope for this crate, we used to have some in crate but they've since been taken out. I think it would be interesting to do experiments with the new Allocator API and memory maps, but ideally it can be done outside this crate (if not, we should fix that).

@danburkert danburkert closed this Aug 17, 2017

@danburkert

This comment has been minimized.

Copy link
Owner

danburkert commented Aug 17, 2017

Woops, there are definitely still refactors coming (see #46).

@danburkert danburkert reopened this Aug 17, 2017

@danburkert danburkert closed this Oct 28, 2017

danburkert added a commit to danburkert/aho-corasick that referenced this issue Oct 29, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

danburkert added a commit to danburkert/aho-corasick that referenced this issue Oct 29, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

danburkert added a commit to danburkert/ripgrep that referenced this issue Oct 29, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

danburkert added a commit to danburkert/wayland-kbd that referenced this issue Oct 29, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

danburkert added a commit to danburkert/fst that referenced this issue Oct 29, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

Note: since `memmap` is a public dependency of `fst` I had to take some
liberties with modifying public interfaces. You probably don't want to
land this PR as is, however I think it will be a useful example for when
the time comes to bump `memmap`.

danburkert added a commit to danburkert/fst that referenced this issue Oct 29, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

Note: since `memmap` is a public dependency of `fst` I had to take some
liberties with modifying public interfaces. You probably don't want to
land this PR as is, however I think it will be a useful example for when
the time comes to move to the new `memmap` API internally.

danburkert added a commit to danburkert/gimli that referenced this issue Oct 29, 2017

Update to memmap 0.6
Update to memmap 0.6

`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

danburkert added a commit to danburkert/tantivy that referenced this issue Oct 29, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

danburkert added a commit to danburkert/block_alloc that referenced this issue Oct 29, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

danburkert added a commit to danburkert/object that referenced this issue Oct 29, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

vberger added a commit to Smithay/wayland-kbd that referenced this issue Oct 29, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

BurntSushi added a commit to BurntSushi/ripgrep that referenced this issue Nov 22, 2017

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

BurntSushi added a commit to BurntSushi/aho-corasick that referenced this issue Jan 1, 2018

Update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

BurntSushi added a commit to BurntSushi/aho-corasick that referenced this issue Jan 1, 2018

deps: update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.

BurntSushi added a commit to BurntSushi/aho-corasick that referenced this issue Jan 1, 2018

deps: update to memmap 0.6
`memmap` 0.6.0 introduces major API changes in anticipation of a 1.0
release. See https://github.com/danburkert/memmap-rs/releases/tag/0.6.0
for more information. CC danburkert/memmap-rs#33.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.