Anim: use a non-allocating array inside CombinedKeyingResult #120425

Nathan Vegdahl · 2024-04-09T13:44:32+02:00

Nathan Vegdahl commented

2024-04-09 13:44:32 +02:00

The data in CombinedKeyingResult was using BLI's Array type, which
heap allocates. However, we know the array size at compile time,
and therefore can use std::array instead, as suggested in Array's
documentation. This makes CombinedKeyingResult a lighter weight
plain-old-data class.

The data in CombinedKeyingResult was using BLI's Array type, which heap allocates. However, we know the array size at compile time, and therefore can use std::array instead, as suggested in Array's documentation. This makes CombinedKeyingResult a lighter weight plain-old-data class.

👍 1

Nathan Vegdahl added 1 commit 2024-04-09 13:44:46 +02:00

29b8286fc8 Anim: use a non-allocating array inside CombinedKeyingResult

The data in CombinedKeyingResult was using BLI's Array type, which
heap allocates.  However, we know the array size at compile time,
and therefore can use std::array instead, as suggested in Array's
documentation.  This makes CombinedKeyingResult a lighter weight
plain-old-data class.

Nathan Vegdahl added the

Module

Animation & Rigging

label 2024-04-09 13:45:01 +02:00

Nathan Vegdahl requested review from Christoph Lendenfeld 2024-04-09 13:45:10 +02:00

Nathan Vegdahl requested review from Sybren A. Stüvel 2024-04-09 13:45:33 +02:00

Nathan Vegdahl commented

2024-04-09 13:47:01 +02:00

This is a small enough change that I was considering just committing to main. But I decided to make it a PR for knowledge-sharing purposes.

Christoph Lendenfeld approved these changes 2024-04-09 15:15:56 +02:00

Christoph Lendenfeld left a comment

looks good and still works as before

Sybren A. Stüvel approved these changes 2024-04-09 16:40:33 +02:00

Sybren A. Stüvel left a comment

👍 for knowledge sharing purposes. Otherwise I think this is a bit of a premature optimisation, where now the code is a bit harder to read. I doubt that this is going to practically speed up key insertion.

I'll leave it to you @nathanvegdahl to see if you still want to land ;-)

:+1: for knowledge sharing purposes. Otherwise I think this is a bit of a premature optimisation, where now the code is a bit harder to read. I doubt that this is going to practically speed up key insertion. I'll leave it to you @nathanvegdahl to see if you still want to land ;-)

👍 1

Hans Goudey reviewed 2024-04-09 16:42:34 +02:00

source/blender/animrig/ANIM_keyframing.hh Outdated

						
				@ -49,3 +50,3 @@

				  /* The index to the array maps a `SingleKeyingResult` to the number of times this result has

				   * occurred. */

				  Array<int> result_counter;

				  std::array<int, static_cast<size_t>(SingleKeyingResult::_KEYING_RESULT_MAX)> result_counter;

Hans Goudey commented

2024-04-09 16:42:34 +02:00

The style guide mentions using functional style cast for this sort of cast: size_t(...

The style guide mentions using functional style cast for this sort of cast: `size_t(...`

👍 1

nathanvegdahl marked this conversation as resolved

Nathan Vegdahl added 1 commit 2024-04-09 17:10:19 +02:00

a21a3d7fe3 Use function-style type cast

Nathan Vegdahl commented

2024-04-09 17:24:50 +02:00

@dr.sybren

Otherwise I think this is a bit of a premature optimisation

Yeah, that's fair. But for me it's less about optimization, and more that (IMO) if something can be POD, it almost always should be. Not for performance, but because POD types have trivial behavior/semantics, and are thus easier to reason about in code.

Additionally, Array's documentation specifically says to use std::array is these cases:

If the size is known at compile time, std::array<T, N> should be used instead.

@dr.sybren > Otherwise I think this is a bit of a premature optimisation Yeah, that's fair. But for me it's less about optimization, and more that (IMO) if something can be POD, it almost always should be. Not for performance, but because POD types have trivial behavior/semantics, and are thus easier to reason about in code. Additionally, `Array`'s documentation specifically says to use `std::array` is these cases: > If the size is known at compile time, `std::array<T, N>` should be used instead.

👍 1

Nathan Vegdahl merged commit 05226881f7 into main

2024-04-09 17:27:29 +02:00

Nathan Vegdahl referenced this issue from a commit

2024-04-09 17:27:30 +02:00

Anim: make CombinedKeyingResult plain old data

Nathan Vegdahl deleted branch combined_keying_result_no_alloc

2024-04-09 17:27:32 +02:00

Raul Fernandez Hernandez referenced this issue from a commit

2024-04-11 22:00:19 +02:00

Anim: make CombinedKeyingResult plain old data

Sign in to join this conversation.