GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — mmap module by killme2008 · Pull Request #3755 · RustPython/RustPython

Conversation

killme2008 commented May 28, 2022 •

edited by fanninpm

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Try to implement mmap module #2059

Supersedes and closes #3564

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

killme2008 commented May 31, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone commented May 31, 2022

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

killme2008 commented May 31, 2022

Copy link

Copy Markdown

Contributor Author

@youknowone Got it, my rust version is older.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fanninpm reviewed

May 31, 2022

View reviewed changes

Comment thread

stdlib/src/mmap.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

fanninpm requested changes

May 31, 2022

View reviewed changes

fanninpm left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

stdlib/src/mmap.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

stdlib/src/mmap.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

stdlib/src/mmap.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

fanninpm commented May 31, 2022

Copy link

Copy Markdown

Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

killme2008 commented Jun 1, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

killme2008 force-pushed the mmap-module branch from ceb8edb to 8fd4640 Compare

June 5, 2022 07:42

killme2008 commented Jun 5, 2022

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

fanninpm commented Jun 5, 2022

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

killme2008 commented Jun 6, 2022

Copy link

Copy Markdown

Contributor Author

@fanninpm Of course, i will break it out.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone requested changes

Jun 6, 2022

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

There was an error while loading. Please reload this page.

qingshi163 reviewed

Jun 6, 2022

View reviewed changes

Comment thread

stdlib/src/mmap.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

qingshi163 reviewed

Jun 6, 2022

View reviewed changes

Comment thread

stdlib/src/mmap.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

killme2008 commented Jun 7, 2022

Copy link

Copy Markdown

Contributor Author

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

youknowone reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

killme2008 commented Jun 9, 2022

Copy link

Copy Markdown

Contributor Author

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

Please review this PR again. Thank you.

youknowone reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

killme2008 requested review from fanninpm and youknowone

June 9, 2022 11:24

fanninpm requested changes

Jun 9, 2022

View reviewed changes

Comment thread

Lib/test/test_mmap.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

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 commented Jun 12, 2022

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 commented Jun 14, 2022

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

fanninpm left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

stdlib/src/mmap.rs Outdated

Comment on lines +39 to +40

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

fanninpm Jun 14, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

stdlib/src/mmap.rs Outdated

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

fanninpm Jun 14, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

stdlib/src/mmap.rs Outdated

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

fanninpm Jun 14, 2022

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone force-pushed the mmap-module branch from 23eb8e2 to 108a9b2 Compare

June 14, 2022 16:01

fanninpm commented Jun 14, 2022

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Basic mmap module

d4f001f

youknowone force-pushed the mmap-module branch from 108a9b2 to fbd79fc Compare

June 14, 2022 16:38

youknowone commented Jun 14, 2022

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone dismissed fanninpm’s stale review

June 14, 2022 17:36

resolved

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 commented Jun 15, 2022

Copy link

Copy Markdown

Contributor Author

@youknowone @fanninpm Thank you for all your helper.

Snowapril reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

qingshi163

qingshi163 left review comments

youknowone

youknowone approved these changes

fanninpm

fanninpm left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

killme2008 commented May 28, 2022 •

edited by fanninpm

Loading

Uh oh!

There was an error while loading. Please reload this page.

killme2008 commented May 31, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

killme2008 commented Jun 1, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.