Skip to content

Improve performance of external (UDR) functions#7989

Merged
asfernandes merged 2 commits intomasterfrom
work/udrperf
Feb 28, 2024
Merged

Improve performance of external (UDR) functions#7989
asfernandes merged 2 commits intomasterfrom
work/udrperf

Conversation

@asfernandes
Copy link
Copy Markdown
Member

@asfernandes asfernandes commented Jan 31, 2024 •
edited
Loading

Test preparation:

recreate function div_udr (
    n1 integer,
    n2 integer
) returns double precision
    external name 'udf_compat!UC_div'
    engine udr;

create domain d_int_check integer check (value > 0);
create domain d_double_check double precision check (value >= 0);

recreate function div_udr2 (
    n1 d_int_check,
    n2 d_int_check
) returns d_double_check
    external name 'udf_compat!UC_div'
    engine udr;

Comparison:

set stats on;

-- 4.4s -> 2.7s
execute block
as
    declare n integer = 5000000;
    declare x integer;
begin
    while (n > 0) do
    begin
        n = n - 1;
        x = div_udr(3, 2);
    end
end;

-- 5.1s -> 3.2s
execute block
as
    declare n integer = 5000000;
    declare x integer;
begin
    while (n > 0) do
    begin
        n = n - 1;
        x = div_udr2(3, 2);
    end
end;

@sim1984
Copy link
Copy Markdown
Contributor

sim1984 commented Jan 31, 2024

Does this only improve the performance of external functions only, or is there an effect for procedures/triggers too?

@asfernandes
Copy link
Copy Markdown
Member Author

asfernandes commented Jan 31, 2024

Does this only improve the performance of external functions only, or is there an effect for procedures/triggers too?

It's mostly about functions only.

There could be a marginal improvement with procedures with parameters with constraints, but I didn't tested it, as procedure/trigger implementation could also be greater improved, but is much more difficult.

@dyemanov dyemanov self-requested a review January 31, 2024 17:56
@asfernandes asfernandes self-assigned this Feb 27, 2024
@asfernandes
Copy link
Copy Markdown
Member Author

asfernandes commented Feb 27, 2024

@dyemanov are you going to review this before merge?

dyemanov
dyemanov reviewed Feb 28, 2024
View reviewed changes
Comment thread src/jrd/ExtEngineManager.cpp
dsc dstDesc = dstDescIt[0];
dstDesc.dsc_address = dstMsg + dstArgOffset;

MOV_move(tdbb, &srcDesc, &dstDesc);
Copy link
Copy Markdown
Member

@dyemanov dyemanov Jan 31, 2024

Choose a reason for hiding this comment

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

In some places we still have the code like this:

if (DSC_EQUIV(src, dst))
  memcpy
else
  MOV_move

Do you think it could improve something? I know we also have the same trick inside CVT_move_common(), but it's two calls deeper in the stack...

Copy link
Copy Markdown
Member Author

@asfernandes asfernandes Feb 28, 2024

Choose a reason for hiding this comment

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

It's already being done in CVT_move_common.
It's not inline, but, I think this is not going to be so relevant.

Comment thread src/jrd/ExtEngineManager.cpp Outdated
*dstNullPtr = 0;
}
else
*dstNullPtr = -1;
Copy link
Copy Markdown
Member

@dyemanov dyemanov Jan 31, 2024

Choose a reason for hiding this comment

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

Maybe add a named constant e.g. MSG_NULL_FLAG = -1 instead of using magic numbers directly in code?

Copy link
Copy Markdown
Member Author

@asfernandes asfernandes Feb 28, 2024

Choose a reason for hiding this comment

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

I think FB_FALSE / FB_TRUE could be used as we used them extensively in similar places.

Comment thread src/jrd/ExtEngineManager.cpp
Parameter* parameter = parameters[argNumber / 2];
defaultValueNode = NULL;
// Iterate over the format items, except the EOF item.
const unsigned paramCount = message->format->fmt_count / 2;
Copy link
Copy Markdown
Member

@dyemanov dyemanov Jan 31, 2024

Choose a reason for hiding this comment

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

Being paranoid, I'd also add an assertion about fmt_count being even ;-)

Copy link
Copy Markdown
Member Author

@asfernandes asfernandes Feb 28, 2024

Choose a reason for hiding this comment

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

It should be wrong. It's used to initialize output message, that is going to have the null flag.
In this case, it purposely ignores it.

Comment thread src/jrd/ExtEngineManager.cpp

// Initialize outputs in the internal message.
{ // scope
fb_assert(udf->getOutputFormat()->fmt_desc.getCount() / 2 == udf->getOutputFields().getCount());
Copy link
Copy Markdown
Member

@dyemanov dyemanov Feb 28, 2024

Choose a reason for hiding this comment

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

I see nothing this scope would protect. Is it really needed?

Copy link
Copy Markdown
Member Author

@asfernandes asfernandes Feb 28, 2024

Choose a reason for hiding this comment

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

I used it mostly as a block to not create a separate function just for it.
I'm not removing the explicit scope label.

Comment thread src/jrd/ExtEngineManager.cpp Outdated
defaultValue = EVL_expr(tdbb, request, fieldInfo.defaultValue);

if (request->req_flags & req_null)
defaultValue = nullptr;
Copy link
Copy Markdown
Member

@dyemanov dyemanov Feb 28, 2024

Choose a reason for hiding this comment

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

IIRC, this condition/assignment is redundant, EVL_expr() is guaranteed to return nullptr in this case.

@asfernandes asfernandes merged commit 547cb83 into master Feb 28, 2024
@asfernandes asfernandes deleted the work/udrperf branch February 28, 2024 21:43
asfernandes added a commit that referenced this pull request Mar 17, 2024
@asfernandes
Postfix for #7989 - Improve performance of external (UDR) functions.
e437721
@asfernandes asfernandes mentioned this pull request Mar 17, 2024
Merged
asfernandes added a commit that referenced this pull request Apr 1, 2024
@asfernandes
Postfix for #7989 - Improve performance of external (UDR) functions (#…
f1c4e00 
…8046)

* Postfix for #7989 - Improve performance of external (UDR) functions.

* Extract common functions.

* Fix profiler request time for external functions.

* Change cancellation detection for external functions.

* Move new private code to anonymous namesapce and rename functions.
dyemanov pushed a commit that referenced this pull request Nov 16, 2024
@asfernandes @dyemanov
Postfix for #7989 - Improve performance of external (UDR) functions (#…
ca9f68f 
…8046)

* Postfix for #7989 - Improve performance of external (UDR) functions.

* Extract common functions.

* Fix profiler request time for external functions.

* Change cancellation detection for external functions.

* Move new private code to anonymous namesapce and rename functions.
@mrotteveel mrotteveel added the rlsnotes60: no Intentionally not added to the Firebird 6.0 release notes. label Mar 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dyemanov dyemanov dyemanov approved these changes

Assignees

@asfernandes asfernandes

Labels

affect-version: 4.0.4 affect-version: 5.0.0 affect-version: 6.0 Initial fix-version: 6.0 Alpha 1 qa: done successfully rlsnotes60: no Intentionally not added to the Firebird 6.0 release notes. type: improvement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@asfernandes @sim1984 @dyemanov @mrotteveel @pavel-zotov