Skip to content

Conversation

@ndossche
Copy link
Member

@ndossche ndossche commented May 1, 2024

Builds upon all previous PRs.

With benchmark from #14076.
Before:

1.5271680355072
1.9233000278473
2.0597560405731

After:

1.4276769161224
1.8403890132904
1.9751319885254

One of the changes here is avoiding initializing and releasing temporary numbers, but only done for the most simple (and common) functions.

Copy link
Member

@SakiTakamachi SakiTakamachi left a comment

Choose a reason for hiding this comment

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

I made one comment, but LGTM

}

bc_add (first, second, &result, scale);
result = bc_add (first, second, scale);
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
result = bc_add (first, second, scale);
bc_num result = bc_add (first, second, scale);

Maybe can declare and initialize it at the same time and remove the result from the cleanup.

But I'm not sure if it affects performance.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I expect this will only make the failure path potentially faster, as it can avoid a call then to the freeing. For the success path I don't think it matters, but I guess feel free to try.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Changes make sense to me.

Please rebase to keep commit history

@ndossche ndossche merged commit 039344c into php:master May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants