Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

mmap module#3755

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
youknowone merged 10 commits intoRustPython:mainfromkillme2008:mmap-module
Jun 14, 2022
Merged

mmap module#3755

youknowone merged 10 commits intoRustPython:mainfromkillme2008:mmap-module
Jun 14, 2022

Conversation

killme2008
Copy link
Contributor

@killme2008killme2008 commentedMay 28, 2022•
edited by fanninpm
Loading

Try to implement mmap module#2059

Supersedes andcloses#3564

@killme2008killme2008 marked this pull request as draftMay 28, 2022 11:19
@killme2008
Copy link
ContributorAuthor

killme2008 commentedMay 31, 2022•
edited
Loading

@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 informationerror[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 informationerror[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
Copy link
Member

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

@killme2008
Copy link
ContributorAuthor

@youknowone Got it, my rust version is older.

Copy link
Contributor

@fanninpmfanninpm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Some of yourlibc imports are causingrustc and/orclippy to complain.

@fanninpm
Copy link
Contributor

There's a test intest_os.py that usesmmap, but that test seems to be Windows-only (which appears to be beyond the scope of this PR).

Can you copytest_mmap.py from CPython 3.10 intoLib/tests? This should give us good code coverage for this new Rust-based module.

@killme2008
Copy link
ContributorAuthor

killme2008 commentedJun 1, 2022•
edited
Loading

@fanninpm thank you. This PR isn't ready for review, it's draft right now. I have some protocols such asAsBuffer ,AsMapping to implement. I will request CR when it's ready,maybe in this weekend.

@killme2008
Copy link
ContributorAuthor

@youknowone@fanninpm
Hi, this PR is ready for code review, alltest_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-8Run tests sequentially0:00:00 load avg: 2.18 [1/1] test_mmaptest_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) ... oktest_anonymous (test.test_mmap.MmapTests) ... oktest_bad_file_desc (test.test_mmap.MmapTests) ... oktest_basic (test.test_mmap.MmapTests) ... oktest_concat_repeat_exception (test.test_mmap.MmapTests) ... oktest_context_manager (test.test_mmap.MmapTests) ... oktest_context_manager_exception (test.test_mmap.MmapTests) ... oktest_crasher_on_windows (test.test_mmap.MmapTests) ... skipped 'requires Windows'test_double_close (test.test_mmap.MmapTests) ... oktest_empty_file (test.test_mmap.MmapTests) ... oktest_entire_file (test.test_mmap.MmapTests) ... oktest_error (test.test_mmap.MmapTests) ... oktest_extended_getslice (test.test_mmap.MmapTests) ... oktest_extended_set_del_slice (test.test_mmap.MmapTests) ... oktest_find_end (test.test_mmap.MmapTests) ... oktest_flush_return_value (test.test_mmap.MmapTests) ... oktest_invalid_descriptor (test.test_mmap.MmapTests) ... skipped 'requires Windows'test_io_methods (test.test_mmap.MmapTests) ... oktest_length_0_large_offset (test.test_mmap.MmapTests) ... oktest_length_0_offset (test.test_mmap.MmapTests) ... oktest_madvise (test.test_mmap.MmapTests) ... oktest_move (test.test_mmap.MmapTests) ... oktest_non_ascii_byte (test.test_mmap.MmapTests) ... oktest_offset (test.test_mmap.MmapTests) ... oktest_prot_readonly (test.test_mmap.MmapTests) ... oktest_read_all (test.test_mmap.MmapTests) ... oktest_read_invalid_arg (test.test_mmap.MmapTests) ... oktest_repr (test.test_mmap.MmapTests) ... oktest_resize_past_pos (test.test_mmap.MmapTests) ... skipped 'resizing not supported'test_rfind (test.test_mmap.MmapTests) ... oktest_sizeof (test.test_mmap.MmapTests) ... skipped 'implementation detail specific to cpython'test_subclass (test.test_mmap.MmapTests) ... oktest_tagname (test.test_mmap.MmapTests) ... skipped 'requires Windows'test_tougher_find (test.test_mmap.MmapTests) ... oktest_weakref (test.test_mmap.MmapTests) ... oktest_write_returning_the_number_of_bytes_written (test.test_mmap.MmapTests) ... ok----------------------------------------------------------------------Ran 40 tests in 0.165sOK (skipped=9)test_mmap passed== Tests result: SUCCESS ==1 test OK.Total duration: 186 msTests result: SUCCESS

But there are still some work to do because ofmemmap2 missing some features:

  • resize method is not supported.
  • flush range is not supported.
  • flags and pro inmmap method is not supported.

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

Please help me review this module, thank you.

@killme2008killme2008 marked this pull request as ready for reviewJune 5, 2022 07:48
@killme2008killme2008 changed the title[WIP] mmap module skeletonmmap moduleJun 5, 2022
@fanninpm
Copy link
Contributor

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

@killme2008
Copy link
ContributorAuthor

@fanninpm Of course, i will break it out.

Copy link
Member

@youknowoneyouknowone left a comment

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.

@killme2008
Copy link
ContributorAuthor

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

youknowone reacted with thumbs up emoji

@killme2008
Copy link
ContributorAuthor

@youknowone I tried to address all the CR problems in3ba2341

Please review this PR again. Thank you.

youknowone reacted with thumbs up emoji

@youknowoneyouknowone mentioned this pull requestJun 10, 2022
@killme2008
Copy link
ContributorAuthor

@fanninpm@youknowone@qingshi163 I tried to address all CR problems again in commiteed1537 and fixedsize() method returns the wrong result(It should return the length of the file, not the length in mmap).

@killme2008
Copy link
ContributorAuthor

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

fanninpm
fanninpm previously requested changesJun 14, 2022
Copy link
Contributor

@fanninpmfanninpm left a comment

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,
Copy link
Contributor

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 ofmemmap2 v0.5.4. If this doesn't work, you may need to have Cargo refresh theCargo.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 thatmemmap2 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,
Copy link
Contributor

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 fromlibc, I recommend searching the.txt files inhttps://github.com/rust-lang/libc to see exactly which platforms are compatible with which constants/methods fromlibc.

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 */
Copy link
Contributor

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 (// ...).

@fanninpm
Copy link
Contributor

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

@youknowone
Copy link
Member

I changed memmap2 version to0.5.4. If we require0.5.4, using0.5 is not a good idea.

Copy link
Member

@youknowoneyouknowone left a comment

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!

@youknowoneyouknowone merged commit1a2451b intoRustPython:mainJun 14, 2022
@fanninpmfanninpm mentioned this pull requestJun 14, 2022
@killme2008
Copy link
ContributorAuthor

@youknowone@fanninpm Thank you for all your helper.

Snowapril reacted with hooray emoji

@killme2008killme2008 deleted the mmap-module branchJune 15, 2022 02:11
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@qingshi163qingshi163qingshi163 left review comments

@youknowoneyouknowoneyouknowone approved these changes

@fanninpmfanninpmfanninpm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@killme2008@youknowone@fanninpm@qingshi163
[8]ページ先頭
©2009-2025 Movatter.jp