pijul_org / pijul

#235 #[repr(packed)] considered harmful

Opened by lthms, on December 7, 2017
Closed
lthms commented on December 7, 2017
13:59 < lthms> oh\r                                                                                                                              +
14:00 < lthms> latest rust does not like the pijul codebase\r                                                                                    +
14:00 < lthms> #[derive] can't be used on a #[repr(packed)] struct with type parameters (error E0133)\r                                          +
14:00 < lthms>  warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!\r+
14:01 < lthms> https://github.com/rust-lang/rust/issues/46043\r                                                                                  +
14:04 < lthms> the question is: do we really need repr(packed)?\r                                                                                +
14:06 < lthms> https://doc.rust-lang.org/nomicon/other-reprs.html#reprpacked\r                                                                   +
14:06 < lthms> “repr(packed) is not to be used lightly. Unless you have extreme requirements, this should not be used.”\r                        +
14:22 < lthms> trying to remove them to see how it goes, just for the fun\r                                                                      +
14:28 < lthms> okay, it looks like pijul still works when removing them\r                                                                        +
14:33 < lthms> but I can’t tell about performance\r                                                                                              +
14:34 < lthms> we should think about tools to measure it, not to compare pijul to other, but to compare one pijul to another\r                   +
```\r                                                                                                                                            +
\r                                                                                                                                               +
@pmeunier can you explain why you use `repr(packed)` in the first place and if you think removing them is a good/bad idea?\r                     +
\r                                                                                                                                               +
Patches are attached to illustrate the drastic solution.
pmeunier commented on December 7, 2017

This is indeed a problem, as constructors like Edge (Edge and Key<PatchId> get written a lot, so this might matter) use this.\r+ \r + One way out of this is to let Rust do whatever it does, and write a test that checks the pointers are as they should be.

lthms commented on December 7, 2017

The thing is, it’ll stop compiling soon enough.

pmeunier commented on December 7, 2017

Right, but we need that. My suggestion is to get rid of #[repr(packed)] everywhere, and replace it with a test of the following form:\r+ \r +

#[test]\r                                                                                                                                +
fn test_repr_edge() {\r                                                                                                                  +
  let e = Edge { flag: EdgeFlags::empty(), dest: ROOT_KEY, introduced_by: ROOT_PATCH };\r                                                +
  assert_eq!(&e.flag as usize, &e as usize);\r                                                                                           +
  assert_eq!((&e.dest as usize) - (&e as usize), 1);\r                                                                                   +
  assert_eq!((&e.introduced_by as usize) - (&e as usize), 17);\r                                                                         +
  assert_eq!(std::mem::size_of::<Edge>(), 26);\r                                                                                         +
}\r                                                                                                                                      +
lthms commented on December 8, 2017

I don’t know, it feels a bit “hacky”, especially because that sort of things are typically what may vary from one system to another and with the use of the --release flag.\r + \r + I would rather (1) try to write something that allows use to evaluate the current performance of one version of pijul and (2) try to compare the impact of getting ride of repr(packed) in a “clean way”.\r+ \r + But to be honest, I am not an expert concerning that kind of things.

pmeunier commented on December 8, 2017

This is not supposed to vary between systems, it's entirely the compiler's choice.\r + Another element to consider is that Sanakirja guarantees that the first byte of each key and the first byte of each value is 64-bit-aligned.\r + \r + However, PatchId, which is a [u8; 8], cannot be stored as a u64, because it would become misaligned in Edge, which has one byte before it and would be 64-bits aligned (because its largest field Key<PatchId> would be).\r+ \r + One thing we can do for now is just to write each field independently.

lthms commented on December 8, 2017

I will trust you on this one.