Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Consider using inline constexpr for the large sEdgeGroupTable table defined in VolumeToMesh.h #1896

Open
jessey-git opened this issue Sep 12, 2024 · 0 comments
Labels

Comments

@jessey-git
Copy link

jessey-git commented Sep 12, 2024

Environment

Operating System: Windows or Linux
Version / Commit SHA: OpenVDB 11
Other: MSVC C++17 or g++

Describe the bug

There is a large 3.2kb table defined in VolumeToMesh.h that will be included in each translation unit that includes this header and the table will remain duplicated in the final executable. There's a few other smaller tables as well.
https://github.com/AcademySoftwareFoundation/openvdb/blob/master/openvdb/openvdb/tools/VolumeToMesh.h#L441

The proper, ODR-safe, way to define such tables inside headers is to use inline constexpr with C++17 onwards.

To Reproduce

  • Create a simple setup, OpenVDB not required, where there's 3 implementation files including a header containing the table
  • Print out the Address of the table in each implementation file
  • Notice that there's 3 different addresses printed, and note the binary size as well

Using the const table:

g++ -O2 -std=c++17 main.cc impl_file1.cc impl_file2.cc
./a.out
main       : 0x555d077d0040
impl file 1: 0x555d077d0d40
impl file 2: 0x555d077d1a40

ls -lt ./a.out
-rwxr-xr-x 1 deadpin deadpin 24416 Sep 11 19:43 a.out

Expected behavior

  • Do the above experiment but use the proper declaration of the table
  • Notice that only 1 address is printed and binary size has been reduced

Using the inline constexpr table:

g++ -O2 -std=c++17 main.cc impl_file1.cc impl_file2.cc
./a.out
main       : 0x55d94e584040
impl file 1: 0x55d94e584040
impl file 2: 0x55d94e584040

ls -lt ./a.out
-rwxr-xr-x 1 deadpin deadpin 20264 Sep 11 19:43 a.out

Additional context

(Add any other context about the problem here.)

@jessey-git jessey-git added the bug label Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant