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

[Core] Improve CowData and Memory metadata alignment. #87814

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Feb 1, 2024

Fixes #87442

This should not change real memory structure on any platform. But instead of hard-coded values, it is using actual sizes and alignments to allocate and access metadata.

@bruvzg bruvzg added this to the 4.3 milestone Feb 1, 2024
@bruvzg bruvzg force-pushed the memalign branch 4 times, most recently from 1c3ed24 to ffc07df Compare February 1, 2024 08:34
@bruvzg
Copy link
Member Author

bruvzg commented Feb 1, 2024

Tested on:

  • building and running on x86_32 Linux (fresh openSUSE VM).
  • building and running on x86_64 (both 32-bit and 64-bit versions, and both MinGW and MSVC).
  • building for x86_32 Android.
  • building and running on ARM64 macOS.

On 32-bit Linux, I'm getting a few failed FastNoiseLite tests, but it seems to be an unrelated floating point precision issue (0.8 != 0.7961) otherwise it seems to be working fine.

Another unrelated note: 32-bit MinGW GCC crash when building with debug symbols on Windows (cross-building is fine), probably symbols are not too big (might be worth adding a note to the compiling page).

@bruvzg bruvzg marked this pull request as ready for review February 1, 2024 10:44
@bruvzg bruvzg requested a review from a team as a code owner February 1, 2024 10:44
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here make a lot of sense to me

core/templates/cowdata.h Outdated Show resolved Hide resolved
core/os/memory.h Outdated Show resolved Hide resolved
Comment on lines 68 to 74
// Alignment: ↓ max_align_t ↓ uint64_t ↓ max_align_t
// ┌─────────────────┬──┬────────────────┬──┬───────────...
// │ uint64_t │░░│ uint64_t │░░│ T[]
// │ alloc size │░░│ element count │░░│ data
// └─────────────────┴──┴────────────────┴──┴───────────...
// Offset: ↑ _size_offset ↑ _elem_offset ↑ _data_offset

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

@@ -46,7 +46,7 @@ class CharString;
template <class T, class V>
class VMap;

SAFE_NUMERIC_TYPE_PUN_GUARANTEES(uint64_t)
static_assert(std::is_trivially_destructible<std::atomic<uint64_t>>::value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this because of the SafeNumeric<uint64_t> around, I'd suggest including this in the set of type-pun guarantees macro in safe_refcount.h.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is part of SAFE_NUMERIC_TYPE_PUN_GUARANTEES:

#define SAFE_NUMERIC_TYPE_PUN_GUARANTEES(m_type)                    \
	static_assert(sizeof(SafeNumeric<m_type>) == sizeof(m_type));   \
	static_assert(alignof(SafeNumeric<m_type>) == alignof(m_type)); \
	static_assert(std::is_trivially_destructible<std::atomic<m_type>>::value);

We take case or sizes and alignment manually (since alignof part is not true on 32-bit platforms), and I have copy-pasted the remaining check here (but I'm not sure if there's any point for checking it for uint64_t at all).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense.

@akien-mga
Copy link
Member

Needs rebase after #87871.

@bruvzg bruvzg force-pushed the memalign branch 2 times, most recently from 4c086be to b5a96b0 Compare February 5, 2024 14:15
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-confirming

core/os/memory.h Show resolved Hide resolved
@akien-mga akien-mga merged commit f8f2c8c into godotengine:master Feb 5, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips
Copy link
Member

Would be good to get a Godot-cpp equivalent fix too

@bruvzg
Copy link
Member Author

bruvzg commented Feb 5, 2024

Would be good to get a Godot-cpp equivalent fix too

Will do it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build fails for linuxbsd x86_32 arch
4 participants