Reduce the size of Mesh, Basis & Quadrature, Direction & Size, ElementId, FixedHashMap, and combine ElementId and DirectionalId #6000

nilsdeppe · 2024-05-16T03:43:01Z

Proposed changes

After looking at memory consumption, @kidder, @wthrowe and I were surprised how much these small classes ended up using. This is because we have a lot of them stored per element. Using smaller integer types greatly reduces the memory usage, and then some bit twiddling is done internally in a few classes to further reduce memory usage. This should also help by reducing memory traffic (being able to load an entire Mesh in 1 load instead of 6, etc.). The bit twiddling is just simple bitmasks so nothing too crazy.

One thing I found quite inconvenient and a bit silly is that we version tiny classes like Direction. We should instead be versioning the Domain during serialization, not all these small classes. In some cases we used more bytes during serialization for the version than the actual data. That seems unnecessary. In cases where I could, I compressed the version info into unused bits by other parts of the class. This helps, but still seems silly.

Upgrade instructions

Code review checklist

The code is documented and the documentation renders correctly. Run
make doc to generate the documentation locally into BUILD_DIR/docs/html.
Then open index.html.
The code follows the stylistic and code quality guidelines listed in the
code review guide.
The PR lists upgrade instructions and is labeled bugfix or
new feature if appropriate.

Further comments

wthrowe · 2024-05-16T17:53:50Z

src/NumericalAlgorithms/Spectral/Basis.hpp

+ * bits of a uint8_t, which is why we do the left shift.  We cannot
+ * have more than 16 bases to fit into the 4 bits, including the
+ * `Uninitialized` value. The number of bits to shift is encoded in the variable
+ * `Spectral::detail::basis_shift`.


detail stuff should not be part of the public interface. Either make the constant non-detail or do the shift internally when using the enum instead of incorporating it into the value.

I don't see how the detail is currently part of the interface. If you are adding or removing values from the enum class, then you need to know why it is being shifted and the limitations, as in, you've entered the realm of "details". If you are just using it, you shouldn't be casting an enum class anyway. Should I make this a \warning or preface it with If you are changing the values in this or something?

You use it in src/IO/H5/VolumeData.cpp.

wthrowe · 2024-05-16T17:56:41Z

src/NumericalAlgorithms/Spectral/Mesh.hpp

+  friend bool operator==(const Mesh<LocalDim>& lhs, const Mesh<LocalDim>& rhs);
+
+  // - 8 bits per extent = 3 * 8 = 24 bits = 3 bytes
+  //   This limits us to 128 extent per direction. Since FD is 2x DG, this


Where does 128 come from?

Heh, probably a typo when computing 2^8. I think the limitation is 255 in practice on FD, which would be a limit of 127 on DG. Does that clarify? (If so, I'll update the comments)

wthrowe · 2024-05-16T18:00:52Z

src/NumericalAlgorithms/Spectral/Mesh.hpp

+  //   This limits us to 128 extent per direction. Since FD is 2x DG, this
+  //   means at most 64 DG points, which is should be fine.
+  // - We encode the basis & quadrature as two sets of 4 bits in a uint8_t.
+  std::array<uint8_t, 6> bit_field_{


I think this would be easier to read if you had two arrays of size 3 instead of one array of size 6 with the first and second halves interpreted differently.

wthrowe · 2024-05-16T18:04:25Z

src/NumericalAlgorithms/Spectral/Mesh.hpp

-  std::array<Spectral::Basis, Dim> bases_{};
-  std::array<Spectral::Quadrature, Dim> quadratures_{};
+  static Spectral::Basis extract_basis(uint8_t bits);
+  static Spectral::Quadrature extract_quadrature(uint8_t bits);


[optional] Bit of a weird asymmetry in that the combining function is in an anonymous namespace but the extracting functions are private statics. I'd make them them all anonymous.

wthrowe · 2024-05-16T18:05:46Z

tests/Unit/NumericalAlgorithms/Spectral/Test_Mesh.cpp

@@ -95,6 +95,7 @@ void test_explicit_choices_per_dimension() {
  test_extents_basis_and_quadrature(mesh1d, {{2}},
                                    {{Spectral::Basis::Legendre}},
                                    {{Spectral::Quadrature::GaussLobatto}});
+  return;
  CHECK(mesh1d.slice_away(0) == Mesh<0>{});


Fix something.

wthrowe · 2024-05-16T18:10:27Z

src/Domain/Structure/Direction.cpp

-  side_ = side;
+template <size_t VolumeDim>
+uint8_t Direction<VolumeDim>::self_bits() {
+  return static_cast<uint8_t>(Axis::Xi) bitor static_cast<uint8_t>(Side::Self);


[optional] return self().bits(); to reduce duplication. (Or even get rid of this function entirely.)

wthrowe · 2024-05-16T18:22:10Z

src/Domain/Structure/ElementId.cpp

+    : block_id_(element_id.block_id_),
+      grid_index_(element_id.grid_index_),
+      // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
+      direction_(*reinterpret_cast<const uint8_t*>(&direction)),


direction_(direction.bits())

src/NumericalAlgorithms/Spectral/Basis.hpp

src/Domain/Structure/Direction.cpp

src/Domain/Structure/Direction.hpp

src/Domain/Structure/ElementId.hpp

src/DataStructures/FixedHashMap.hpp

Reduces size of serialized objects.

nilsdeppe · 2024-05-20T23:44:22Z

Rebased and pushed fixups. Thanks for the reviews!

kidder

streaming in the ASSERT is not working on the failing build...

otherwise you have my permission to squash once you have the okay from @wthrowe

wthrowe

Squash.

wthrowe · 2024-05-21T00:20:08Z

src/NumericalAlgorithms/Spectral/Mesh.cpp

-  p | bit_field_;
+  p | extents_;
+  p | quadrature_and_basis_;
+  // p | bit_field_;


wthrowe · 2024-05-21T00:20:19Z

src/NumericalAlgorithms/Spectral/Mesh.cpp

@@ -274,7 +276,9 @@ bool is_isotropic(const Mesh<Dim>& mesh) {

 template <size_t Dim>
 bool operator==(const Mesh<Dim>& lhs, const Mesh<Dim>& rhs) {
-  return lhs.bit_field_ == rhs.bit_field_;
+  // return lhs.bit_field_ == rhs.bit_field_;


This reduces the size of Mesh from 48 bytes to 6 bytes. We store the mesh quite a few places and it ends up being around 1kB per element at 48 bytes. Also moves various functions that needed editing into the cpp file.

This needs to be one commit because we version these tiny classes.

nilsdeppe · 2024-05-21T14:29:45Z

Okay, done! I verified that the test failure is fixed locally so 🤞

nilsdeppe requested review from wthrowe and kidder May 16, 2024 03:43

wthrowe requested changes May 16, 2024

View reviewed changes

nilsdeppe force-pushed the mesh_tweaks branch from cadf2f7 to 2052511 Compare May 19, 2024 23:14

kidder requested changes May 20, 2024

View reviewed changes

nilsdeppe added 2 commits May 20, 2024 18:40

Add check that the size of Mesh is what we expect

c1ce30a

Make Basis & Quadrature enums uint8_t

28a4c94

Reduces size of serialized objects.

nilsdeppe force-pushed the mesh_tweaks branch from 2052511 to 7c8dc06 Compare May 20, 2024 23:44

kidder reviewed May 20, 2024

View reviewed changes

wthrowe requested changes May 21, 2024

View reviewed changes

nilsdeppe added 8 commits May 21, 2024 09:58

Use a compressed bit representation for the Mesh

4b627c9

This reduces the size of Mesh from 48 bytes to 6 bytes. We store the mesh quite a few places and it ends up being around 1kB per element at 48 bytes. Also moves various functions that needed editing into the cpp file.

Fix formating in ElementId.hpp

b791a53

Cleanup includes in Domain.hpp

9c00316

Compress Direction and Side sizes

44dbbe5

This needs to be one commit because we version these tiny classes.

Compress ElementId and add support for a Direction in it

50c0c65

Reduce size type in FixedHashMap

a13ba6a

Combine ElementId and DirectionalId

872ba05

Fix clang-tidy warnings in Test_SimpleWenoImpl

336ba5a

nilsdeppe force-pushed the mesh_tweaks branch from 7c8dc06 to 336ba5a Compare May 21, 2024 14:29

kidder approved these changes May 21, 2024

View reviewed changes

wthrowe approved these changes May 21, 2024

View reviewed changes

wthrowe merged commit 6bd9cb2 into sxs-collaboration:develop May 21, 2024
21 of 22 checks passed

nilsdeppe deleted the mesh_tweaks branch August 6, 2024 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the size of Mesh, Basis & Quadrature, Direction & Size, ElementId, FixedHashMap, and combine ElementId and DirectionalId #6000

Reduce the size of Mesh, Basis & Quadrature, Direction & Size, ElementId, FixedHashMap, and combine ElementId and DirectionalId #6000

nilsdeppe commented May 16, 2024

wthrowe May 16, 2024

nilsdeppe May 16, 2024

wthrowe May 16, 2024

wthrowe May 16, 2024

nilsdeppe May 16, 2024

wthrowe May 16, 2024

wthrowe May 16, 2024

wthrowe May 16, 2024

wthrowe May 16, 2024

wthrowe May 16, 2024

nilsdeppe commented May 20, 2024

kidder left a comment

wthrowe left a comment

wthrowe May 21, 2024

wthrowe May 21, 2024

nilsdeppe commented May 21, 2024

Reduce the size of Mesh, Basis & Quadrature, Direction & Size, ElementId, FixedHashMap, and combine ElementId and DirectionalId #6000

Reduce the size of Mesh, Basis & Quadrature, Direction & Size, ElementId, FixedHashMap, and combine ElementId and DirectionalId #6000

Conversation

nilsdeppe commented May 16, 2024

Proposed changes

Upgrade instructions

Code review checklist

Further comments

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsdeppe commented May 20, 2024

kidder left a comment

Choose a reason for hiding this comment

wthrowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilsdeppe commented May 21, 2024