Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite boilerplate impls into macro #41

Closed
wants to merge 3 commits into from
Closed

Rewrite boilerplate impls into macro #41

wants to merge 3 commits into from

Conversation

@ImmemorConsultrixContrarie
Copy link
Contributor

@ImmemorConsultrixContrarie ImmemorConsultrixContrarie commented Jan 23, 2020

Two problems:

  1. I accidentally rustfmt it;
  2. Usize type now has TYPENAME "usize" instead of "u32" or "u64". It could get back by implementing this trait for usize as a standalone with some "cfg!()"s.
Two problems:
1) I accidentally rustfmt it;
2) Usize type now has TYPENAME "usize" instead of "u32" or "u64". It could get back by implementing this trait for usize as a standalone with some "cfg!()"s.
@ImmemorConsultrixContrarie
Copy link
Contributor Author

@ImmemorConsultrixContrarie ImmemorConsultrixContrarie commented Jan 23, 2020

Oops... Actually, I wanted to make a draft PR, as it's not really complete: some comments should be deleted and some things in this code look really strange to me. For example, why do you need all this "as_slice", "from_slice", if you can just write

use core::intrinsics::copy_nonoverlapping;

let mut buf = ToType::some_default_zero();
let Vsize = mem::size_of::<ValueType>();
let Tsize = mem::size_of::<ToType>();
unsafe {
  if Tsize > Vsize {
    // Some endian check, dunno if it should be cfged or checked in runtime.
    if cfg!(target_endian = "little") {
      copy_nonoverlapping(&value as *const u8, &mut buf as *mut u8, Vsize);
    } else {
      copy_nonoverlapping(&value as *const u8, (&mut buf).add(Tsize-Vsize)  as *mut u8, Vsize);
    }
  } else {
    // Tsize <= Vsize
    if cfg!(target_endian = "little") {
      copy_nonoverlapping(&value as *const u8, &mut buf as *mut u8, Tsize);
    } else {
      copy_nonoverlapping((&value as *const u8).add(Vsize-Tsize), &mut buf  as *mut u8, Vsize);
    }
  }
}
return buf;
Rustfmt'ed again, whatever.
Tested, all tests pass (except for those in "../traits.rs/fmt!" macro, they simply didn't compile (I dunno if those are tests or not at all).

Now that unsafe is pretty strait, clean and should const-eval into one line actually.
fn resize<T, U>(value: T) -> U
where T: BitStore, U: BitStore {
let zero = 0usize;
let mut slab = zero.to_ne_bytes();
let start = 0;

/* Copy the source value into the correct region of the intermediate slab.
The `BitStore::as_bytes` method returns the value as native-endian-order
bytes. These bytes are then written into the correct location of the slab
(low addresses on little-endian, high addresses on big-endian) to be
interpreted as `usize`.
*/
match mem::size_of::<T>() {
n @ 1 | n @ 2 | n @ 4 | n @ 8 => {
#[cfg(target_endian = "big")]
let start = mem::size_of::<usize>() - n;

slab[start ..][.. n].copy_from_slice(value.as_bytes());
},
_ => unreachable!("BitStore is not implemented on types of this size"),
}
let mid = usize::from_ne_bytes(slab);
// Truncate to the correct size, then wrap in `U` through the trait method.
match mem::size_of::<U>() {
1 => U::from_bytes(&(mid as u8).to_ne_bytes()[..]),
2 => U::from_bytes(&(mid as u16).to_ne_bytes()[..]),
4 => U::from_bytes(&(mid as u32).to_ne_bytes()[..]),
#[cfg(target_pointer_width = "64")]
8 => U::from_bytes(&mid.to_ne_bytes()[..]),
_ => unreachable!("BitStore is not implemented on types of this size"),
}
where
T: BitStore,
U: BitStore,
{
use core::intrinsics::copy_nonoverlapping;

let mut buf = U::all_bits_zeroes();
let size_of_from = mem::size_of::<T>();
let size_of_into = mem::size_of::<U>();
unsafe {
if cfg!(target_endian = "big") {
// Big endian fills from the end.
if size_of_from > size_of_into {
// T has more bytes, move its pointer to match the length of U.
copy_nonoverlapping(
(&value as *const T as *const u8).add(size_of_from - size_of_into),
&mut buf as *mut U as *mut u8,
size_of_from,
);
} else {
// U has more bytes, move its pointer to match the length of T.
// Won't move pointer if sizes are equal.
copy_nonoverlapping(
&value as *const T as *const u8,
(&mut buf as *mut U as *mut u8).add(size_of_into - size_of_from),
size_of_from,
);
}
} else {
// Little endian fills from the start.
// Fill buffer with bits from start.
// All unfilled bytes are zeroes, cuz
// `let mut buf = U::all_bits_zeroes();`
if size_of_from > size_of_into {
copy_nonoverlapping(
&value as *const T as *const u8,
&mut buf as *mut U as *mut u8,
size_of_into,
);
} else {
copy_nonoverlapping(
&value as *const T as *const u8,
&mut buf as *mut U as *mut u8,
size_of_from,
);
}
}
}

buf
}
Comment on lines 947 to 998

This comment has been minimized.

@ImmemorConsultrixContrarie

ImmemorConsultrixContrarie Jan 23, 2020
Author Contributor

As commit says: "Now that unsafe is pretty strait, clean and should const-eval into one line actually."
That's the only notable change actually. Big-endian should be tested, but I don't think there would be any problems.
Deleted things that were commented out, and as_bytes, from_bytes were replaced with all_bits_zeroes. The same thing could be done with MaybeUninit::zeroed, but this way is safer and should have the same effectiveness.

@ImmemorConsultrixContrarie
Copy link
Contributor Author

@ImmemorConsultrixContrarie ImmemorConsultrixContrarie commented Jan 23, 2020

Travis failed with "could not find stringify in core"... This macro is stable since 1.0.0, wtf?

src/fields.rs Outdated Show resolved Hide resolved
@ImmemorConsultrixContrarie
Copy link
Contributor Author

@ImmemorConsultrixContrarie ImmemorConsultrixContrarie commented Jan 23, 2020

Btw, why not simply make 3-word wide BitMut

pub struct BitMut<'a, O, T>
where
    O: BitOrder,
    T: 'a + BitStore,
{
    /// A reference to a single bit in memory.
    pub(super) slot: &'a mut BitSlice<O, T>,
    /// A local cache for `Deref` usage.
    pub(super) data: bool,
}

into something like this (two words wide):

pub struct BitMut<'a, O, T>
where
    O: BitOrder,
    T: 'a + BitStore,
{
    // Tell the compiler, this struct holds mutable access of the slice.
    _slice: core::marker::PhantomData<&'a mut BitSlice<O, T>>,
    // Writer.
    if cfg!(feature = "atomic") {
    word: AtomicPtr<T>,
    } else {
      word: *mut T,
    }
    // Derefed data, writes on drop as it does now.
    data: bool,
    // A mask-creator.
    place: u8, // or `u16`, but until `u256` would be a thing, u16 is overkill
}

Or make "split_mut" create a tuple of (&mut self, BitSliceWithOneClonedStorage), so this thing doesn't even need atomics, won't has atomics overhead, and could be Send at all times.

@myrrlyn
Copy link
Collaborator

@myrrlyn myrrlyn commented Jan 23, 2020

Thank you for the PR. I appreciate your willingness to find improvements as well as defects. Other than the copy-size note, I have nothing to really put in the review workflow for the contents of the changes, but here's what I have for the PR as a whole:

src/fields.rs

My fields::resize implementation was a second draft, and I stopped working on it as soon as I got to a useful point. The pointer-arithmetic version you wrote is cleaner, has no annoying edge cases in source code, and removes the detritus I had added to BitStore in order to support byte-based resizing. With the right copy length, it passes the test suite on my machine, and I'm happy to include it.

src/store.rs

I also appreciate the macro collapse of the store trait implementations. I don't know why the path core::stringify! doesn't resolve, but as the macro is in the prelude of both core and std, it doesn't need the prefix in either no_std or std modes.

I've removed the all_bits_zero constructor function, as it is equivalent to the FALSE constant. fields::resize uses U::FALSE as the constructor instead of U::all_bits_zero, with identical behavior.

I am ambivalent on whether usize should rename to a fixed-size integer or not; the TYPENAME constant is now only used in formatting, the rename probably does not need to occur. I, personally, prefer fixed-size type names in the debug information, but I don't think it matters enough to warrant a change.

Project Environment

I apologize for the extremely out-of-rustfmt style of the repository, and the lack of a rustfmt.toml to make rustfmt behave. This is the second contribution I've had, and the first from someone I didn't personally know and talk to out-of-band, so I just haven't set one up. I'll set up a rustfmt.toml and run cargo fmt on the repository in the near future, so that future contributors don't run into the hassle you did dealing with the formatting noise.

Resolution

I've reshaped your PR's actual changes with the minor surface differences that I would apply after accepting your PR; however, I would prefer that you retain credit for the lines you changed in git blame and that my syntactic differences don't paper them over.

I've attached a file 41.diff.txt with the end result of applying your branch and reshaping it. (GitHub requires the .txt extension, unfortunately). If you are comfortable with this diff, please overwrite your current PR branch with it. You may also feel free to add your name to AUTHORS.txt if you wish. Otherwise, I'll merge it as-is and resolve the formatting differences later.

Thank you again for writing this; the new fields::resize implementation is much better than my original, and boilerplate reduction in the store traits is nice from a maintenance perspective.

@ImmemorConsultrixContrarie
Copy link
Contributor Author

@ImmemorConsultrixContrarie ImmemorConsultrixContrarie commented Jan 23, 2020

prefer fixed-size type names in the debug information

Welp, I'd describe writing "usize" as "u64" or "u32" as a mistake, even though it is a renamed unsigned int. Also, you can always size_of::<usize>() to get the real size in bytes for format.

And I don't wanna do all this "apply other's changes from diff file" work instead of real coding, so feel free to write whatever changes you did. I'd only suggest adding "unsafe" to the "Sealed" trait and writing "unsafe" in macro implementing it.

@myrrlyn
Copy link
Collaborator

@myrrlyn myrrlyn commented Jan 23, 2020

You're right about BitMut; after I realized I couldn't make an actual fake-reference type that commits on drop (Rust forbids impl Drop for &mut _, even though it's an affine !Copy type), I stopped thinking about how to optimize it and went back to working on the rest of the stdlib-compatibility tasks.

The structure

struct BitMut<'a, O, T>
where O: BitOrder, T: BitStore {
  _parent: PhantomData<&'a mut BitSlice<O, T>>,
  data: NonNull<T::Access>,
  head: BitIdx<T>,
  bit: bool,
}

contains all the information needed to operate, and fits in two words. Thanks for the input!

As the pointer itself is never modified, its slot does not need to be atomic.

@ImmemorConsultrixContrarie
Copy link
Contributor Author

@ImmemorConsultrixContrarie ImmemorConsultrixContrarie commented Jan 23, 2020

As the pointer itself is never modified, its slot does not need to be atomic.

IIRC, AtomicPtr has atomic access to the data.

@myrrlyn
Copy link
Collaborator

@myrrlyn myrrlyn commented Jan 23, 2020

It does not; it is equivalent to an AtomicUsize except that its value is a memory address. AtomicPtr::load returns a *mut T, which has ordinary non-atomic derefence behavior.

Which is really irritating, because it does look like the kind of name you'd give to a pointer with permanent Relaxed dereference semantics.

myrrlyn added a commit that referenced this pull request Jan 23, 2020
myrrlyn added a commit that referenced this pull request Jan 23, 2020
Suggested by GitHub user @ImmemorConsultrixContrarie in PR #41, this
change structures the `BitMut` proxy reference as a memory pointer,
bit index, and bit cache. This structure fits within two words
instead of three (a full `&mut BitSlice` is not necessary, as the
structure only ever refers to one bit in one element).

This is purely a stack optimization; it has no effect on the
language-level inability to use it as the reference value in
ordinary access operations.
@myrrlyn
Copy link
Collaborator

@myrrlyn myrrlyn commented Jan 23, 2020

I'm going to close this, as I just applied the rollup commit to master.

@myrrlyn myrrlyn closed this Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants