Use non-zero optimization for Generation #447

andrewhickman · 2018-08-04T20:11:50Z

As promised in issue #436, this PR uses NonZeroU32 in Generation. Note this bumps the minimum supported rust version to 1.28 - is this a concern? If so I could put this under a feature flag.

This change is

torkleyy · 2018-08-04T20:17:17Z

Hi! Unfortunately, this is a nightly-only feature, we cannot use this optimization.

andrewhickman · 2018-08-04T20:21:53Z

NonZeroU32 was stabilized a couple days ago with rust 1.28.0.

torkleyy

Ah, now you removed the feature requirement. I think this makes sense, but I don't want it to be merged yet because 1.28 is the latest version. Can we come back to this in a few weeks?

Aceeri · 2018-09-14T21:34:18Z

@torkleyy @andrewhickman 1.29 is released now, might be worth looking at rebasing/merging this (unless we want a larger buffer size).

Xaeroxe · 2018-09-20T15:54:22Z

Should probably be released in 0.13, I'm doing a 0.12.3 right now

pthariensflame · 2018-09-20T17:42:17Z

src/world/entity.rs

@@ -383,20 +406,38 @@ impl Generation {
    /// Panics in debug mode if it's not alive.
    fn die(&mut self) {
        debug_assert!(self.is_alive());
-        self.0 = -self.0;
+        self.0 = NonZeroU32::new(-self.id() as u32).map(Generation);


Can't this underflow and therefore panic in debug builds?

Sure can, so yeah that should be corrected.

Which operation can overflow?

@andrewhickman The negation.

@andrewhickman Or alternately, the cast. I'm not sure where the problem would actually occur.

The assert should ensure that id() is positive and the only overflow case is where id() is i32::MIN so I think this one is correct. AFAIK signed-ness casts are always no-ops in rust so that should be OK as well

Ok; I can accept the assert! making the negation safe, but I'd like to see where casts from signed to unsigned are supposed to be defined as bitcasts.

Never mind; found it: https://doc.rust-lang.org/reference/expressions/operator-expr.html#semantics

~~id() can only ever be a positive number by virtue of the fact that it is built from a NonZeroU32. Won't this always panic?~~

scratch that, I was missing context

Hm ok but it can only ever be positive or 0. I think we still have a problem.

pthariensflame · 2018-09-20T18:25:32Z

src/world/entity.rs

+    /// Panics if it is alive.
+    fn raised(&self) -> Generation {
+        assert!(!self.is_alive());
+        Generation(unsafe { NonZeroU32::new_unchecked((1 - self.id()) as u32) })


Another very similar possible underflow here.

Note that wrapping will give the wrong behaviour here, sending id from i32::MIN + 1 to i32::MIN and vice versa. I think it would be better to just unconditionally panic here. Potentially we could start wrapping the generations back around to 1 but that sounds risky to me.

… overflow

andrewhickman · 2018-09-20T19:47:38Z

The test failure seems to be caused by ab43ce1

Xaeroxe · 2018-09-20T20:05:17Z

I removed is_alive() and raised() from the generation because if a Generation can no longer be less than 0 then it's not possible for that structure to register as "dead". Since a structure can never be dead what's the point of "check if this is alive and if so return it". Perhaps I missed something but is that not what a NonZeroU32 does?

Xaeroxe · 2018-09-20T20:24:10Z

Alright, after having read all the overflow/underflow bits and pieced this together I understand now why what I did broke the PR. I'm not super sure though that the loss in readability is worth the minute performance gains to be had from this.

Xaeroxe · 2018-09-20T20:25:30Z

@andrewhickman would you rather I put the PR back together or would you prefer to?

Xaeroxe · 2018-09-20T20:35:37Z

I don't know, I'm not even sure it's worth the effort to be honest. Specs is hard enough to read as is, and while this is technologically an improvement relying on overflow behavior to implement negatives makes it so much harder to maintain that I question the value of this.

Xaeroxe · 2018-09-20T20:51:05Z

Ok, so I've been thinking about this a bit more and I actually see a way we can make this work. You've already made sure that the only entry and exit points from the Generation structure output to i32 correctly, the next thing I feel this really needs to be perfect is that everywhere we're casting to a NonZeroU32 or to a i32 we should provide a comment explaining the overflow/underflow behavior. This way we still have a Generation value with a nice interface, and specs maintainers understand what's going on.

andrewhickman · 2018-09-20T21:16:59Z

@Xaeroxe not sure from your comments if you worked this out but the only reason I used NonZeroU32 is that NonZeroI32 was never stabilised. So the inner value can be negative, but the bits are stored as u32. I'll add a comment explaining this and fix this PR up tomorrow.

Xaeroxe · 2018-09-20T21:17:54Z

Yeah I learned that from the original issue... probably should have read that first :P Sorry for all the trouble, and thanks for fixing my mistakes

pthariensflame · 2018-09-20T23:14:56Z

If we wanted to go a little overboard, we could implement a NonZeroI32 on stable as a #[repr(transparent)] wrapper around NonZeroU32, delegating methods and trait implementations wherever possible.

Xaeroxe · 2018-09-20T23:35:33Z

Maybe we ought to make a crate ;)

This reverts commit ab43ce1.

This reverts commit c002016.

This reverts commit dc39760.

Xaeroxe · 2018-09-21T19:27:23Z

@pthariensflame K you're gonna laugh but...

I did it! https://crates.io/crates/nonzero_signed

andrewhickman · 2018-09-21T20:30:06Z

@Xaeroxe ooh nice! Shall we use it here?

Xaeroxe · 2018-09-21T20:30:40Z

Might as well :)

…into non-zero-gen

Xaeroxe · 2018-09-21T21:34:25Z

Thank you!

torkleyy reviewed Aug 5, 2018

View reviewed changes

torkleyy added the postponed label Sep 7, 2018

andrewhickman added 2 commits September 16, 2018 00:03

use non-zero optimization for Generation

95d8c87

Prepare for pull request

9f857e5

Xaeroxe added 4 commits September 20, 2018 11:11

Merge branch 'master' into non-zero-gen

e361f44

Remove unusable function

dc39760

Remove assert which is now always true

c002016

Remove unnecessary mappings

ab43ce1

pthariensflame reviewed Sep 20, 2018

View reviewed changes

add a better Debug impl for Generation and always panic on generation…

4c99a93

… overflow

andrewhickman added 5 commits September 21, 2018 18:27

Revert "Remove unnecessary mappings"

7ddcde8

This reverts commit ab43ce1.

Revert "Remove assert which is now always true"

0097864

This reverts commit c002016.

Revert "Remove unusable function"

024d941

This reverts commit dc39760.

Re-add Generation::is_alive

5b6afe6

Add comment explaining why Generation uses NonZeroU32

b375df0

Merge branch 'master' into non-zero-gen

ab57246

andrewhickman added 2 commits September 21, 2018 21:39

Use NonZeroI32 from nonzero_signed crate

2182192

Merge branch 'non-zero-gen' of https://github.com/andrewhickman/specs …

8e42018

…into non-zero-gen

Xaeroxe merged commit 5c445f3 into amethyst:master Sep 21, 2018

andrewhickman deleted the non-zero-gen branch September 21, 2018 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use non-zero optimization for Generation #447

Use non-zero optimization for Generation #447

andrewhickman commented Aug 4, 2018 •

edited by torkleyy

Loading

torkleyy commented Aug 4, 2018

andrewhickman commented Aug 4, 2018

torkleyy left a comment

Aceeri commented Sep 14, 2018 •

edited

Loading

Xaeroxe commented Sep 20, 2018

pthariensflame Sep 20, 2018

Xaeroxe Sep 20, 2018

andrewhickman Sep 20, 2018

pthariensflame Sep 20, 2018

pthariensflame Sep 20, 2018

andrewhickman Sep 20, 2018

pthariensflame Sep 20, 2018

pthariensflame Sep 20, 2018

Xaeroxe Sep 20, 2018 •

edited

Loading

Xaeroxe Sep 20, 2018

pthariensflame Sep 20, 2018

andrewhickman Sep 20, 2018

andrewhickman commented Sep 20, 2018 •

edited

Loading

Xaeroxe commented Sep 20, 2018

Xaeroxe commented Sep 20, 2018

Xaeroxe commented Sep 20, 2018

Xaeroxe commented Sep 20, 2018

Xaeroxe commented Sep 20, 2018

andrewhickman commented Sep 20, 2018

Xaeroxe commented Sep 20, 2018

pthariensflame commented Sep 20, 2018 •

edited

Loading

Xaeroxe commented Sep 20, 2018

Xaeroxe commented Sep 21, 2018

andrewhickman commented Sep 21, 2018

Xaeroxe commented Sep 21, 2018 via email •

edited

Loading

Xaeroxe commented Sep 21, 2018

Use non-zero optimization for Generation #447

Use non-zero optimization for Generation #447

Conversation

andrewhickman commented Aug 4, 2018 • edited by torkleyy Loading

torkleyy commented Aug 4, 2018

andrewhickman commented Aug 4, 2018

torkleyy left a comment

Choose a reason for hiding this comment

Aceeri commented Sep 14, 2018 • edited Loading

Xaeroxe commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xaeroxe Sep 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewhickman commented Sep 20, 2018 • edited Loading

Xaeroxe commented Sep 20, 2018

Xaeroxe commented Sep 20, 2018

Xaeroxe commented Sep 20, 2018

Xaeroxe commented Sep 20, 2018

Xaeroxe commented Sep 20, 2018

andrewhickman commented Sep 20, 2018

Xaeroxe commented Sep 20, 2018

pthariensflame commented Sep 20, 2018 • edited Loading

Xaeroxe commented Sep 20, 2018

Xaeroxe commented Sep 21, 2018

andrewhickman commented Sep 21, 2018

Xaeroxe commented Sep 21, 2018 via email • edited Loading

Xaeroxe commented Sep 21, 2018

andrewhickman commented Aug 4, 2018 •

edited by torkleyy

Loading

Aceeri commented Sep 14, 2018 •

edited

Loading

Xaeroxe Sep 20, 2018 •

edited

Loading

andrewhickman commented Sep 20, 2018 •

edited

Loading

pthariensflame commented Sep 20, 2018 •

edited

Loading

Xaeroxe commented Sep 21, 2018 via email •

edited

Loading