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.

Sign up for GitHub

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

fixed memory leak on x86 android devices. #17172

Merged
merged 3 commits into from
Jun 18, 2024
Merged

fixed memory leak on x86 android devices. #17172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8622f9c
fixed memory leak on x86 android devices.
dumganhar Jun 17, 2024
cdebb3c
Update GFXDef-common.h
dumganhar Jun 17, 2024
72b2210
Update native-compile-platforms.yml
dumganhar Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/native-compile-platforms.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,9 @@ jobs:
sed -i "s@^COCOS_ENGINE_PATH.*@COCOS_ENGINE_PATH=$NATIVE_ROOT@g" gradle.properties
sed -i "s@^NATIVE_DIR.*@NATIVE_DIR=$NATIVE_DIR@g" gradle.properties

# To speedup CI, compile arm64-v8a only
# To speedup CI, compile x86 only
# sed -i "s@^PROP_APP_ABI.*@PROP_APP_ABI=armeabi-v7a:arm64-v8a:x86@g" gradle.properties
sed -i "s@^PROP_APP_ABI.*@PROP_APP_ABI=arm64-v8a@g" gradle.properties
sed -i "s@^PROP_APP_ABI.*@PROP_APP_ABI=x86@g" gradle.properties

echo "Compile Android - cmake ..."
echo "ANDROID_NDK ${ANDROID_NDK} or ${ANDROID_NDK_HOME}"
Expand Down
4 changes: 4 additions & 0 deletions native/cocos/base/Macros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,3 +409,7 @@ It should work same as apples CFSwapInt32LittleToHost(..)
#define CC_FORCE_INLINE inline
#endif
#endif

// Macro definition to check if a type has padding
#define CC_CHECK_STRUCT_NO_PADDING(T) \
static_assert(std::has_unique_object_representations_v<T>, "ERROR: Type has padding bytes!")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can not use std::has_unique_object_representations_v<T> to check padding, as in the following codes, it also returns false

struct D {
    int* ptr;
};

struct E {
    int id;
    std::string name;
    double* data;
};

Will return false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could work for primitve types, and all gfx structs that need the added padding manually could work by std:: has_unique_object_representations_v

So do we need to consider stl containers, like string, vector, list, map in struct?
And if so , any suggestion for how to check those types?

Copy link
Contributor Author

@dumganhar dumganhar Jun 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The example you wrote:

struct D {
    int* ptr;
};

returns true.

You could paste code

#include <type_traits>
#include <stdint.h>
#include <iostream>
#include <cstddef> 

#define CHECK_NO_PADDING(T) \
    static_assert(std::has_unique_object_representations_v<T>, "Type " #T " has padding bytes!")


struct D {
    int* ptr;
};

struct E {
    int id;
    std::string name;
    double* data;
};

int main() {

  
  CHECK_NO_PADDING(D); // return true, since int* is 4 bytes,  the default padding is 4 bytes too.
  CHECK_NO_PADDING(E);  // return false, 
   

    return 0;
}

in cpp.sh to test. The environment on cpp.sh is x86 .

Copy link
Contributor

Choose a reason for hiding this comment

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

So do we need to consider stl containers, like string, vector, list, map in struct?
And if so , any suggestion for how to check those types?

It depends on the usage. But as this function is defined in Macros.h, so should add comments to say that it only works for primitive types.

Copy link
Contributor

Choose a reason for hiding this comment

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

struct D {
int* ptr;
};
returns true.

Yep, i was deceived by ChatGPT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a comment later.

10 changes: 7 additions & 3 deletions native/cocos/renderer/gfx-base/GFXDef-common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1291,7 +1291,9 @@ struct ALIGNAS(8) ColorAttachment {
LoadOp loadOp{LoadOp::CLEAR};
StoreOp storeOp{StoreOp::STORE};
GeneralBarrier *barrier{nullptr};

#if CC_CPU_ARCH == CC_CPU_ARCH_32
uint32_t _padding{0};
#endif
EXPOSE_COPY_FN(ColorAttachment)
};

Expand All @@ -1305,7 +1307,9 @@ struct ALIGNAS(8) DepthStencilAttachment {
LoadOp stencilLoadOp{LoadOp::CLEAR};
StoreOp stencilStoreOp{StoreOp::STORE};
GeneralBarrier *barrier{nullptr};

#if CC_CPU_ARCH == CC_CPU_ARCH_32
uint32_t _padding{0};
#endif
EXPOSE_COPY_FN(DepthStencilAttachment)
};

Expand Down Expand Up @@ -1396,7 +1400,7 @@ struct ALIGNAS(8) BufferBarrierInfo {

uint32_t offset{0};
uint32_t size{0};

uint32_t _padding{0};
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot the padding changed member initialization order. It can be problematic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we depend on the order？

Copy link
Contributor

Choose a reason for hiding this comment

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

I have searched the codebase and it seems aggregate initialization is not used.
So it might be fine.

uint64_t discardContents{0}; // @ts-boolean

Queue *srcQueue{nullptr}; // @ts-nullable
Expand Down
1 change: 1 addition & 0 deletions native/cocos/renderer/gfx-base/GFXDef.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ namespace gfx {
// T must have no implicit padding
template <typename T>
ccstd::hash_t quickHashTrivialStruct(const T *info, size_t count = 1) {
CC_CHECK_STRUCT_NO_PADDING(T);
static_assert(std::is_trivially_copyable<T>::value && sizeof(T) % 8 == 0, "T must be 8 bytes aligned and trivially copyable");
return ccstd::hash_range(reinterpret_cast<const uint64_t *>(info), reinterpret_cast<const uint64_t *>(info + count));
}
Expand Down
Loading