Skip to content

Conversation

@TreeHunter9
Copy link
Contributor

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 by MemPool::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 schemaSearchPath static, 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%.

@TreeHunter9
Copy link
Contributor Author

I wanna add @asfernandes as a reviewer, but it seems I don't have permissions for that.

@aafemt
Copy link
Contributor

aafemt commented Nov 6, 2025

Cannot ObjectArray<MetaString> be made constexpr instead?

I understand that this is a hack for gbak only but still having static variable for schema path here smells.

@aafemt
Copy link
Contributor

aafemt commented Nov 6, 2025

BTW, what's difference between new NonLazyInitInstance and old GlobalPtr?

@AlexPeshkoff
Copy link
Member

If different gbak restore instances may have different schema paths use of static variable for it is very bad idea.
What about storing appropriate pointer in attachment?

@aafemt
Copy link
Contributor

aafemt commented Nov 6, 2025

Isn't schema search path already a property of attachment?..

@AlexPeshkoff
Copy link
Member

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.

@aafemt
Copy link
Contributor

aafemt commented Nov 6, 2025

Perhaps you are looking at single commit of two.

@TreeHunter9
Copy link
Contributor Author

Cannot ObjectArray<MetaString> be made constexpr instead?

No, ObjectArray use a memory allocation for every element, so it's not possible to make it constexpr.

BTW, what's difference between new NonLazyInitInstance and old GlobalPtr?

I need to pass arguments to ctor, but GlobalPtr does not have this option, and I don't want to change the old code (maybe I should) to avoid breaking anything.

@aafemt
Copy link
Contributor

aafemt commented Nov 6, 2025

No, ObjectArray use a memory allocation for every element, so it's not possible to make it constexpr.

Is it unavoidable? std::array is able to be constexpr.

@TreeHunter9
Copy link
Contributor Author

No, ObjectArray use a memory allocation for every element, so it's not possible to make it constexpr.

Is it unavoidable? std::array is able to be constexpr.

I know, but MET_qualify_existing_name() expects ObjectArray, and this is an .epp file, so we can't pass std::array easily, we need some kind of std::span, but now passing ObjectArray will become impossible. It's very clunky, and I don't think it's worth it.

@TreeHunter9
Copy link
Contributor Author

Also, I don't quite understand why some variables and parameters related to the schema are MetaString, while others are MetaName, and they are converting back and forth wasting time. Shouldn't they all just beMetaName @asfernandes?

@aafemt
Copy link
Contributor

aafemt commented Nov 6, 2025

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.

@asfernandes
Copy link
Member

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.

@asfernandes
Copy link
Member

Or even better, add a new contructor to GlobalPtr to receive a lambda initializer. Something like:

GlobalPtr<MyContainer> myContainer(
    [] (auto& container)
    {
        container.push(...);
    }
);

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});
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const T* get() const noexcept
const T* get() const noexcept

@asfernandes asfernandes merged commit 9edccee into FirebirdSQL:master Nov 7, 2025
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants