ENH: Added mmapmodule.c to Allow mmap objects to access the madvise() system call

[a-deeb]

Adding python/cpython#6172 Allow mmap objects to access the madvise() system call , based on @seberg recommendation ,and adding it numpy separately based on issue #13172
I added the mmapmodule.c module in the numpy/numpy/core/src/common/ directory

[charris]

Blank line before.

[charris]

Lots of style nits, more than I commented on. Take a look at doc/C_STYLE_GUIDE.rst.txt.

[charris]

Is this placement safe?

[eric-wieser]

Can't we add just the madvise function, rather than cloning a builtin module?

[seberg]

Yeah, if we put this in for madvise, then we should limit it to the single feature we actually need.

EDIT: Sorry to add it later, the original issue has some ctypes solution. Now we may need to do something more portable and could possibly hide it away in C, but I think that solution seemed about right for a minimal approach.

[mattip]

What is the license of this file? Having author and contributor attribution with no copyright release is IMO problematic, but I am not a lawyer. Where did it come from? Also, do we allow github names as contributors?

[eric-wieser]

I think it's a complete copy of the one in cpython

[mattip]

The original header in cpython is different. If it is copied from there it I think it should at least maintain the original header, probably with the addition of this sentence from the CPython LICENSE "Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019 Python Software Foundation; All Rights Reserved". Looking around at other files in the repo, that would seem to be enough.

[a-deeb]

yes, it is from CPython , I will find out what the license of this file is

[a-deeb]

Hi everyone, I would like to know if there is anything else I could do on this file regarding this issue, let me know, I'm really excited about helping out 😄

[seberg]

@a-deeb well, we did comment above that this should be a minimal port of functionality. We do not need to copy paste all of the memorymap implementation from cpython probably? So to move this along, would it be possible to steal only the parts that we would actually have to put into numpy?

[a-deeb]

@seberg so I should just have the mmap_madvise_method() along with any of its dependent structs and methods and remove everything else? I'll get working that, Thanks again!

[eric-wieser]

I'm not really sure this approach is worth pursuing - it might be best just to wait for the upstream support, and to point users to the ctypes solution until then.

Alternatively, we could include the ctypes solution directly into memmap.py. I don't thing a C wrapper is going to be particularly useful here, especially not if the approach is to try and copy mmap_module, of which almost none is useful.

[seberg]

@a-deeb sorry, but I do not think we will be merging this with the whole of mmap_module.c if you can do a minimal version (maybe best using c-types to keep maintainability high), that would sound like it might go anyway. I appreciate the input, but just want to avoid you putting in a lot of effort in a direction which is not going to get merged in any case. If you do not have time/wish to explore the ctypes solution, it is also fine to close this. Thanks either way!

[seberg]

@a-deeb thanks for the PR, but I will close this for now. Would be happy about a PR implementing a more minimal approach to exposing this in memmap.