mmap module by killme2008 · Pull Request #3755 · RustPython/RustPython

killme2008 · 2022-05-28T11:19:28Z

Try to implement mmap module #2059

Supersedes and closes #3564

killme2008 · 2022-05-31T05:10:53Z

@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?

youknowone · 2022-05-31T05:12:35Z

build errors not reproduced by CI is mostly rust version problem.
try rustup update

killme2008 · 2022-05-31T05:13:48Z

@youknowone Got it, my rust version is older.

stdlib/src/mmap.rs

fanninpm

Some of your libc imports are causing rustc and/or clippy to complain.

stdlib/src/mmap.rs

fanninpm · 2022-05-31T19:43:52Z

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.

killme2008 · 2022-06-01T02:28:44Z

@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.

killme2008 · 2022-06-05T07:48:45Z

@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:

resize method is not supported.
flush range is not supported.
flags and pro in mmap method is not supported.

And i don't implement this module on windows platform too.

Please help me review this module, thank you.

fanninpm · 2022-06-05T18:18:09Z

Can you break out test_mmap.py into its own separate commit? This helps us keep track of the tests more easily.

killme2008 · 2022-06-06T04:11:29Z

@fanninpm Of course, i will break it out.

youknowone

Great! You made a big feature! I left a few comments.

stdlib/Cargo.toml

stdlib/src/mmap.rs

vm/src/lib.rs

stdlib/src/mmap.rs

killme2008 · 2022-06-07T02:17:36Z

@youknowone @fermian @qingshi163 Thank you for all your comments, i will resolve them ASAP.

killme2008 · 2022-06-09T11:23:42Z

@youknowone I tried to address all the CR problems in 3ba2341

Please review this PR again. Thank you.

Lib/test/test_mmap.py

killme2008 · 2022-06-12T07:47:45Z

@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).

killme2008 · 2022-06-14T03:26:42Z

@youknowone I rebased the commits , split the test_mmap.py out into a seperate commit and merge some commits. Please check it, thank you.

fanninpm

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.

fanninpm · 2022-06-14T13:44:57Z

stdlib/src/mmap.rs

+            #[cfg(target_os = "linux")]
+            libc::MADV_FREE => Advice::Free,


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.

Suggested change

#[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.)

fanninpm · 2022-06-14T13:57:09Z

stdlib/src/mmap.rs

+    #[cfg(target_os = "linux")]
+    #[pyattr]
+    use libc::{
+        MADV_DODUMP, MADV_DOFORK, MADV_DONTDUMP, MADV_DONTFORK, MADV_FREE, MADV_HUGEPAGE,


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.

fanninpm · 2022-06-14T14:00:21Z

stdlib/src/mmap.rs

+                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 */


Very minor nitpick: These comments (/* ... */) are inconsistent with the other comments in this file (// ...).

fanninpm · 2022-06-14T16:16:54Z

cargo update -p memmap2 should fix some of the CI failures.

youknowone · 2022-06-14T16:39:17Z

I changed memmap2 version to 0.5.4. If we require 0.5.4, using 0.5 is not a good idea.

youknowone

Great! Thank you for the long time effort!

resolved

killme2008 · 2022-06-15T02:05:52Z

@youknowone @fanninpm Thank you for all your helper.

killme2008 marked this pull request as draft May 28, 2022 11:19

killme2008 force-pushed the mmap-module branch from 248cc3d to bb58931 Compare May 31, 2022 05:05

fanninpm reviewed May 31, 2022

View reviewed changes

stdlib/src/mmap.rs Outdated Show resolved Hide resolved

fanninpm requested changes May 31, 2022

View reviewed changes

stdlib/src/mmap.rs Show resolved Hide resolved

stdlib/src/mmap.rs Show resolved Hide resolved

stdlib/src/mmap.rs Show resolved Hide resolved

killme2008 force-pushed the mmap-module branch from ceb8edb to 8fd4640 Compare June 5, 2022 07:42

killme2008 marked this pull request as ready for review June 5, 2022 07:48

killme2008 force-pushed the mmap-module branch from 137973f to b429732 Compare June 5, 2022 09:19

killme2008 changed the title ~~[WIP] mmap module skeleton~~ mmap module Jun 5, 2022

youknowone requested review from qingshi163 and youknowone June 5, 2022 10:24

youknowone requested changes Jun 6, 2022

View reviewed changes

qingshi163 reviewed Jun 6, 2022

View reviewed changes

stdlib/src/mmap.rs Outdated Show resolved Hide resolved

qingshi163 reviewed Jun 6, 2022

View reviewed changes

stdlib/src/mmap.rs Outdated Show resolved Hide resolved

killme2008 requested review from fanninpm and youknowone June 9, 2022 11:24

fanninpm requested changes Jun 9, 2022

View reviewed changes

Lib/test/test_mmap.py Outdated Show resolved Hide resolved

youknowone mentioned this pull request Jun 10, 2022

wrap_index without abs #3769

Merged

killme2008 requested a review from fanninpm June 12, 2022 07:44

killme2008 force-pushed the mmap-module branch from eed1537 to caa662a Compare June 12, 2022 07:53

killme2008 force-pushed the mmap-module branch from 46453dc to 199e25e Compare June 14, 2022 03:23

killme2008 and others added 2 commits June 14, 2022 16:37

Refactor byte operations

396812d

mmap skeleton

cc4583e

youknowone force-pushed the mmap-module branch 2 times, most recently from b5e9c1f to a93435d Compare June 14, 2022 09:29

youknowone requested a review from fanninpm June 14, 2022 09:32

youknowone force-pushed the mmap-module branch from a93435d to 23eb8e2 Compare June 14, 2022 09:46

fanninpm previously requested changes Jun 14, 2022

View reviewed changes

youknowone force-pushed the mmap-module branch from 23eb8e2 to 108a9b2 Compare June 14, 2022 16:01

Basic mmap module

d4f001f

youknowone force-pushed the mmap-module branch from 108a9b2 to fbd79fc Compare June 14, 2022 16:38

killme2008 and others added 7 commits June 15, 2022 01:59

Impl madvise method

d96b165

Impl read, read_byte and readline methods

ba33bc9

Impl seek, write, write_byte methods etc.

929ad30

Impl AsBuffer, AsMapping and AsSequence protocols for PyMmap

e5a7679

Adds test_mmap.py from CPython 3.10.4

3217de7

Skip test_flush_return_value on linux platform

1ff3e0a

use usize for mmap fields

af2fac9

youknowone force-pushed the mmap-module branch from fbd79fc to af2fac9 Compare June 14, 2022 16:59

youknowone approved these changes Jun 14, 2022

View reviewed changes

youknowone merged commit 1a2451b into RustPython:main Jun 14, 2022

fanninpm mentioned this pull request Jun 14, 2022

mmap is not implemented #2059

Closed

killme2008 deleted the mmap-module branch June 15, 2022 02:11

fanninpm mentioned this pull request Jun 29, 2022

[EPIC] PIP IS BROKEN 🛬💥😭 #3829

Closed

youknowone mentioned this pull request Jul 8, 2022

mmap module for windows #3847

Closed

Conversation

killme2008 commented May 28, 2022 • edited by fanninpm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

killme2008 commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowone commented May 31, 2022

Uh oh!

killme2008 commented May 31, 2022

Uh oh!

Uh oh!

fanninpm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fanninpm commented May 31, 2022

Uh oh!

killme2008 commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

killme2008 commented Jun 5, 2022

Uh oh!

fanninpm commented Jun 5, 2022

Uh oh!

killme2008 commented Jun 6, 2022

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

killme2008 commented Jun 7, 2022

Uh oh!

killme2008 commented Jun 9, 2022

Uh oh!

Uh oh!

killme2008 commented Jun 12, 2022

Uh oh!

killme2008 commented Jun 14, 2022

Uh oh!

fanninpm left a comment

Choose a reason for hiding this comment

Uh oh!

fanninpm Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

fanninpm Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

fanninpm Jun 14, 2022

Choose a reason for hiding this comment

Uh oh!

fanninpm commented Jun 14, 2022

Uh oh!

youknowone commented Jun 14, 2022

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

killme2008 commented Jun 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

killme2008 commented May 28, 2022 •

edited by fanninpm

Loading

killme2008 commented May 31, 2022 •

edited

Loading

killme2008 commented Jun 1, 2022 •

edited

Loading