mmap module by killme2008 · Pull Request #3755 · RustPython/RustPython
@youknowone I found that it can't be compiled in main branch with such error:
error[E0658]: function pointer casts are not allowed in constant functions
--> vm/src/types/slot.rs:248:42
|
248 | length: if has_length { Some(length) } else { None },
| ^^^^^^
|
= note: see issue #57563 <https://github.com/rust-lang/rust/issues/57563> for more information
error[E0658]: function pointer casts are not allowed in constant functions
--> vm/src/types/slot.rs:249:48
|
249 | subscript: if has_subscript { Some(subscript) } else { None },
| ^^^^^^^^^
|
= note: see issue #57563 <https://github.com/rust-lang/rust/issues/57563> for more information
error[E0658]: function pointer casts are not allowed in constant functions
--> vm/src/types/slot.rs:251:22
|
251 | Some(ass_subscript)
Do i miss something?
build errors not reproduced by CI is mostly rust version problem.
try rustup update
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of your libc imports are causing rustc and/or clippy to complain.
There's a test in test_os.py that uses mmap, but that test seems to be Windows-only (which appears to be beyond the scope of this PR).
Can you copy test_mmap.py from CPython 3.10 into Lib/tests? This should give us good code coverage for this new Rust-based module.
@fanninpm thank you. This PR isn't ready for review, it's draft right now. I have some protocols such as AsBuffer , AsMapping to implement. I will request CR when it's ready,maybe in this weekend.
@youknowone @fanninpm
Hi, this PR is ready for code review, all test_mmap tests pass:
RUST_BACKTRACE=1 cargo run --release -- -m test test_mmap -v
Compiling rustpython-stdlib v0.1.2 (/Users/dennis/programming/rust/RustPython/stdlib)
Compiling rustpython v0.1.2 (/Users/dennis/programming/rust/RustPython)
Finished release [optimized] target(s) in 8.11s
Running `target/release/rustpython -m test test_mmap -v`
== RustPython 3.10.0alpha (heads/mmap-module-dirty:18e3402c9, Jun 5 2022, 14:11:57) [rustc 1.61.0]
== macOS-12.3.1-arm64-64bit little-endian
== cwd: /private/var/folders/7p/y52r9hf128sdvqx8rjfrlhs80000gn/T/test_python_43733
== CPU count: 10
== encodings: locale=UTF-8, FS=utf-8
Run tests sequentially
0:00:00 load avg: 2.18 [1/1] test_mmap
test_around_2GB (test.test_mmap.LargeMmapTests) ... skipped 'test requires 6442450944 bytes and a long time to run'
test_around_4GB (test.test_mmap.LargeMmapTests) ... skipped 'test requires 6442450944 bytes and a long time to run'
test_large_filesize (test.test_mmap.LargeMmapTests) ... skipped 'test requires 6442450944 bytes and a long time to run'
test_large_offset (test.test_mmap.LargeMmapTests) ... skipped 'test requires 6442450944 bytes and a long time to run'
test_access_parameter (test.test_mmap.MmapTests) ... ok
test_anonymous (test.test_mmap.MmapTests) ... ok
test_bad_file_desc (test.test_mmap.MmapTests) ... ok
test_basic (test.test_mmap.MmapTests) ... ok
test_concat_repeat_exception (test.test_mmap.MmapTests) ... ok
test_context_manager (test.test_mmap.MmapTests) ... ok
test_context_manager_exception (test.test_mmap.MmapTests) ... ok
test_crasher_on_windows (test.test_mmap.MmapTests) ... skipped 'requires Windows'
test_double_close (test.test_mmap.MmapTests) ... ok
test_empty_file (test.test_mmap.MmapTests) ... ok
test_entire_file (test.test_mmap.MmapTests) ... ok
test_error (test.test_mmap.MmapTests) ... ok
test_extended_getslice (test.test_mmap.MmapTests) ... ok
test_extended_set_del_slice (test.test_mmap.MmapTests) ... ok
test_find_end (test.test_mmap.MmapTests) ... ok
test_flush_return_value (test.test_mmap.MmapTests) ... ok
test_invalid_descriptor (test.test_mmap.MmapTests) ... skipped 'requires Windows'
test_io_methods (test.test_mmap.MmapTests) ... ok
test_length_0_large_offset (test.test_mmap.MmapTests) ... ok
test_length_0_offset (test.test_mmap.MmapTests) ... ok
test_madvise (test.test_mmap.MmapTests) ... ok
test_move (test.test_mmap.MmapTests) ... ok
test_non_ascii_byte (test.test_mmap.MmapTests) ... ok
test_offset (test.test_mmap.MmapTests) ... ok
test_prot_readonly (test.test_mmap.MmapTests) ... ok
test_read_all (test.test_mmap.MmapTests) ... ok
test_read_invalid_arg (test.test_mmap.MmapTests) ... ok
test_repr (test.test_mmap.MmapTests) ... ok
test_resize_past_pos (test.test_mmap.MmapTests) ... skipped 'resizing not supported'
test_rfind (test.test_mmap.MmapTests) ... ok
test_sizeof (test.test_mmap.MmapTests) ... skipped 'implementation detail specific to cpython'
test_subclass (test.test_mmap.MmapTests) ... ok
test_tagname (test.test_mmap.MmapTests) ... skipped 'requires Windows'
test_tougher_find (test.test_mmap.MmapTests) ... ok
test_weakref (test.test_mmap.MmapTests) ... ok
test_write_returning_the_number_of_bytes_written (test.test_mmap.MmapTests) ... ok
----------------------------------------------------------------------
Ran 40 tests in 0.165s
OK (skipped=9)
test_mmap passed
== Tests result: SUCCESS ==
1 test OK.
Total duration: 186 ms
Tests result: SUCCESS
But there are still some work to do because of memmap2 missing some features:
resizemethod is not supported.- flush range is not supported.
- flags and pro in
mmapmethod is not supported.
And i don't implement this module on windows platform too.
Please help me review this module, thank you.
[WIP] mmap module skeleton
mmap module
Can you break out test_mmap.py into its own separate commit? This helps us keep track of the tests more easily.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! You made a big feature! I left a few comments.
@youknowone I tried to address all the CR problems in 3ba2341
Please review this PR again. Thank you.
@fanninpm @youknowone @qingshi163 I tried to address all CR problems again in commit eed1537 and fixed size() method returns the wrong result(It should return the length of the file, not the length in mmap).
@youknowone I rebased the commits , split the test_mmap.py out into a seperate commit and merge some commits. Please check it, thank you.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Just a few quick changes.
Even if anyone else hasn't already said this, thank you for contributing a significant amount of time and effort on this.
Comment on lines +39 to +40
| #[cfg(target_os = "linux")] | ||
| libc::MADV_FREE => Advice::Free, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also present on macOS and iOS, as of memmap2 v0.5.4. If this doesn't work, you may need to have Cargo refresh the Cargo.lock file.
| #[cfg(target_os = "linux")] | |
| libc::MADV_FREE => Advice::Free, | |
| #[cfg(any(target_os = "linux", target_vendor = "apple"))] | |
| libc::MADV_FREE => Advice::Free, |
(Please note that memmap2 does not currently support BSD systems, as the upstream maintainer has not currently set up CI for them.)
| #[cfg(target_os = "linux")] | ||
| #[pyattr] | ||
| use libc::{ | ||
| MADV_DODUMP, MADV_DOFORK, MADV_DONTDUMP, MADV_DONTFORK, MADV_FREE, MADV_HUGEPAGE, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MADV_FREE is also present on Apple devices (and the four major BSDs).
I suggest using something like this (with applicable formatting):
#[cfg(any(target_os = "android", target_os = "dragonfly", target_os = "fuchsia", target_os = "freebsd", target_os = "linux", target_os = "netbsd", target_os = "openbsd", target_vendor = "apple"))] #[pyattr] use libc::MADV_FREE;
As for the other imports from libc, I recommend searching the .txt files in https://github.com/rust-lang/libc to see exactly which platforms are compatible with which constants/methods from libc.
Comment on lines +820 to +830
| 0 => dist, /* relative to start */ | ||
| 1 => { | ||
| /* relative to current position */ | ||
| let pos = self.pos(); | ||
| if (((isize::MAX as usize) - pos) as isize) < dist { | ||
| return Err(vm.new_value_error("seek out of range".to_owned())); | ||
| } | ||
| pos as isize + dist | ||
| } | ||
| 2 => { | ||
| /* relative to end */ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor nitpick: These comments (/* ... */) are inconsistent with the other comments in this file (// ...).
cargo update -p memmap2 should fix some of the CI failures.
I changed memmap2 version to 0.5.4. If we require 0.5.4, using 0.5 is not a good idea.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thank you for the long time effort!