-
-
Notifications
You must be signed in to change notification settings - Fork 262
refactor(vio): Initialize schema search path statically in VIO_store() to remove unnecessary memory allocations
#8800
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
refactor(vio): Initialize schema search path statically in VIO_store() to remove unnecessary memory allocations
#8800
Conversation
|
I wanna add @asfernandes as a reviewer, but it seems I don't have permissions for that. |
|
Cannot I understand that this is a hack for gbak only but still having static variable for schema path here smells. |
|
BTW, what's difference between new |
|
If different gbak restore instances may have different schema paths use of static variable for it is very bad idea. |
|
Isn't schema search path already a property of attachment?.. |
|
Yes, there is one, but looks like gbak needs one more (I did not look deep into this code). BTW, I do not see NonLazyInitInstance in src. |
|
Perhaps you are looking at single commit of two. |
No,
I need to pass arguments to ctor, but |
Is it unavoidable? std::array is able to be constexpr. |
I know, but |
|
Also, I don't quite understand why some variables and parameters related to the schema are |
|
Perhaps better solution would be to use the schema search path from the attachment here, as in any other place, and set the path explicitly in gbak or implicitly during gbak attach. |
|
Please don't create another hack for initialize variables. We have more than the enough of them. It's fine to use a const static, it's const anyway. You can probably initialize it like IntlManager.cpp's ModulesMap, with a constructor that inserts the elements and a GlobalPtr. |
|
Or even better, add a new contructor to GlobalPtr to receive a lambda initializer. Something like: |
src/jrd/vio.cpp
Outdated
| ObjectsArray<MetaString> schemaSearchPath({SYSTEM_SCHEMA, PUBLIC_SCHEMA}); | ||
| static const GlobalPtr<ObjectsArray<MetaString>> schemaSearchPath([]() | ||
| { | ||
| return FB_NEW_POOL(*getDefaultMemoryPool()) ObjectsArray<MetaString>(*getDefaultMemoryPool(), {SYSTEM_SCHEMA, PUBLIC_SCHEMA}); |
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.
Since you prefered your lambda to construct the instance, it should receive a pool parameter and use it.
It looks very weird to the lambda use getDefaultMemoryPool() directly.
| { | ||
| return instance; | ||
| } | ||
| const T* get() const noexcept |
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.
| const T* get() const noexcept | |
| const T* get() const noexcept |
Recently, I have comparing flamegraphs for gbak restore v5 vs v6, found that VIO_store takes longer in v6 than in v5.
About 25% of the execution time of
VIO_store()was taken up byMemPool::allocate()and~ObjectsArray(). As I can see these function calls related to new schema code, so they are not present in the v5 flame graph.I have made
schemaSearchPathstatic, and these calls was removed from flamegraphs, and restore speed for my test table (create table tab1(v1 varchar(25), v2 varchar(25), v3 varchar(25), v4 varchar(25));) increased by ~5%.