fn fguv_32x32xn_rust: fix performance regression introduced in 6751f65c

[fbossen]

No description provided.

[kkysen]

Do you know which commit in #541 caused regression (6751f65 is #541)?

[fbossen]

Do you know which commit in #541 caused regression (6751f65 is #541)?

I've been tracking performance only on the main branch so far. Thus, I don't know which precise commit in #541 is at issue here.

[fbossen]

Looks like there are two commits that lead to performance regression: fc5dbe2 and 999a18d

[kkysen]

Looks like there are two commits that lead to performance regression: fc5dbe2 and 999a18d

Hmm, that's pretty weird. fc5dbe2 I didn't really change anything except move everything into a closure, but the code is doing the same thing. Maybe it wasn't inlined? Do you know how to mark a closure as #[inline]? And for 999a18d, there shouldn't be any bounds checks since it's indexing into an array of length 2 where the index is casted from a bool, so I'm not sure what would cause a slowdown. Let me take a look at the assembly, or do you have other ideas for why it's slower?

[thedataking]

I didn't really change anything except move everything into a closure

I'm not sure what specifically caused the slowdown here but in general, it is harder for a compiler to optimize across function boundaries. It is nice that Frank was able to fix the regression while keeping the closure in this case. However, sometimes it is perfectly fine not to eliminate duplication especially on hot code paths. Once you need three copies of something, it's usually time to deduplicate.

[fbossen]

For fc5dbe2 adding move to the closure definition mostly addresses the issue. I don't think there is a way to define a closure as #[inline] (at least not yet). For 999a18d, I was seeing a compare with 2 in the assembly code. Not sure why the compiler isn't able to figure out that the variable can only take value 0 to 1.

[kkysen]

I didn't really change anything except move everything into a closure

I'm not sure what specifically caused the slowdown here but in general, it is harder for a compiler to optimize across function boundaries. It is nice that Frank was able to fix the regression while keeping the closure in this case. However, sometimes it is perfectly fine not to eliminate duplication especially on hot code paths. Once you need three copies of something, it's usually time to deduplicate.

I added the closure to match the C, which used a macro here, and it was duplicated 4 times each (and it's a good sized chunk of code). Perhaps an inner fn with #[inline(always)] would work better (that should be near guaranteed to be inlined).

[kkysen]

For 999a18d, I was seeing a compare with 2 in the assembly code. Not sure why the compiler isn't able to figure out that the variable can only take value 0 to 1.

The fix in this PR doesn't do anything with offsets from 999a18d, but it changes uv indexing into length-2 arrays. Is that the regression? That would make more sense to me, since the bool to usize cast is happening outside of the fn, and this PR moves it back inside it. I can rewrite things to make the arg bool and do the as usize cast inside the fn, like I do for some other variables, and make sure to do this for other similar patterns in the future.

[fbossen]

I can rewrite things to make the arg bool and do the as usize cast inside the fn, like I do for some other variables, and make sure to do this for other similar patterns in the future.

That would most likely work. I was just going for the fewest changes here. The as usize cast may have to be done inside the closure.

[kkysen]

I can rewrite things to make the arg bool and do the as usize cast inside the fn, like I do for some other variables, and make sure to do this for other similar patterns in the future.

That would most likely work. I was just going for the fewest changes here. The as usize cast may have to be done inside the closure.

Okay, I'll check what works. If the argument is limited to 0 and 1, I'd prefer to make the argument a bool from the start.

As for benchmarking, you're doing this on aarch64-unknown-linux-gnu, right? I didn't notice any non-trivial (e.x. different registers used, a couple of extra non-branching instructions) differences in the asm for fguv_32x32xn_rust when move was added to the add_noise_uv closure, and it looked like the closure was fully inlined (the only bls were to panicking fns).

[fbossen]

As for benchmarking, you're doing this on aarch64-unknown-linux-gnu, right?

aarch64 on macOS

[kkysen]

As for benchmarking, you're doing this on aarch64-unknown-linux-gnu, right?

aarch64 on macOS

Oh yeah. I'm not sure how to cross-compile to aarch64-apple-darwin, though, so hopefully aarch64-unknown-linux-gnu should be close enough since this code shouldn't be OS-specific.

I just opened #559 to fix the uv bounds checks by making the args bools.

Are you sure the closure is the problem here and not just the bounds checks? For me, on x86_64-unknown-linux-gnu and aarch64-unknown-linux-gnu, the add_noise_{y,uv} closures are inlined. Only add_noise_uv has the uv indexing, though, and it's only there that there was a perf regression, right?

[kkysen]

So attributes like #[inline] are unstably supported on closures and other statements:

• fixed/added in Allow #[inline] on closures rust-lang/rust#50273
• tracking issue: Tracking issue for stmt_expr_attributes: Add attributes to expressions, etc. rust-lang/rust#15701

That's been unstable for a long time, though.

[randomPoison]

@kkysen is #559 meant to supersede this PR? If I'm understanding this correctly, both are attempting to address the same performance regression, but #559 does so more cleanly/directly?

[kkysen]

@kkysen is #559 meant to supersede this PR? If I'm understanding this correctly, both are attempting to address the same performance regression, but #559 does so more cleanly/directly?

Yeah, #559 and #560 are meant to supercede this PR by fixing the same performance regressions more cleanly/thoroughly. I'll close this one now.