-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fixed memory leak on x86 android devices. #17172
Conversation
Interface Check ReportThis pull request does not change any public interfaces ! |
@cocos-robot run test cases |
@@ -1396,7 +1400,7 @@ struct ALIGNAS(8) BufferBarrierInfo { | |||
|
|||
uint32_t offset{0}; | |||
uint32_t size{0}; | |||
|
|||
uint32_t _padding{0}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@dumganhar, Please check the result of
Task Details
|
@dumganhar, Please check the result of
Task Details
|
|
||
// 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!") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 .
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Re: #
https://forum.cocos.org/t/topic/158470
Changelog
Continuous Integration
This pull request:
Compatibility Check
This pull request: