Skip to content

Use NonNull<u8> in MmapRegionBuilder for saving space #224

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FuuuOverclocking
Copy link

Summary of the PR

Updates the raw_ptr field in MmapRegionBuilder (mmap_unix.rs) from Option<*mut u8> to Option<NonNull<u8>>. This modification saves 8 bytes on x86-64 systems, for example.

The public API and functionality remain unchanged.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@FuuuOverclocking
Copy link
Author

FuuuOverclocking commented Apr 18, 2023

This commit caused the test to fail due to fn test_mmap_region_build_raw() (vm_memory::mmap_unix::tests) as addr is 0 in this test.

I'm wondering, is addr being 0 a valid parameter? Since the addr here is an HVA, it seems inappropriate to set it to 0. Instead, it could be set to, for example, 40960.

@andreeaflorescu
Copy link
Member

@FuuuOverclocking what is the reason for this PR? 8 bytes in the great scheme of things does not feel like a lot.

@FuuuOverclocking
Copy link
Author

In Rust, using Option<NonNull<u8>> is a common pattern when both Option and pointer are needed.

@epilys
Copy link
Member

epilys commented Oct 12, 2023

@FuuuOverclocking Hello, I think this PR should also wrap MmapRegion::addr in NonNull<_> as well. Would you be interested in rebasing and making this change?

Updates the `raw_ptr` field in `MmapRegionBuilder` (mmap_unix.rs) from
`Option<*mut u8>` to `Option<NonNull<u8>>`. This modification saves
8 bytes on x86-64 systems, for example.

The public API and functionality remain unchanged.

Signed-off-by: Fuu <q.for.xyz@qq.com>
@roypat
Copy link
Member

roypat commented Nov 6, 2023

This seems reasonable to me, I don't think there's any use case in mapping in the zero page, and Option<NonNull<u8>> is indeed the more rust-y approach.

I also agree with @epilys that if we do this, we should probably be consistent with it, and use NonNull<u8> everywhere (but in this case, this probably will need a changelog entry).

Also, it currently seems like unittests are failing, could you look into that? :o

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants