Skip to content
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

Implement BCMath LRU cache #14107

Closed
wants to merge 2 commits into from
Closed

Implement BCMath LRU cache #14107

wants to merge 2 commits into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented May 1, 2024

This is a prototype for an LRU cache for operands used in BCMath.

We notice in profiles that parsing operands takes a long time (like over 30% for the benchmark in #14076).
This PR implements a cache for the 16 most recently used operands, such that parsing can be avoided. The underlying idea is that in a particular time window the same operands are likely to be used again (*).

This implementation could probably be improved too performance-wise.
Important: One notable thing missing is that results are not stored in the LRU cache yet.

(*) The question is whether this is true in real-world use-cases. If this is not true most of the time, then a patch like this will cause a performance degradation because maintaining the LRU also has some overhead.

@nielsdos
Copy link
Member Author

nielsdos commented May 1, 2024

cc @Girgias @SakiTakamachi Let me know if you're aware of real-world test cases we could try this on and what your opinion on something like this is.

@SakiTakamachi
Copy link
Member

A typical use case for BCMath is money calculations.

That is, if BCMath needs to convert the same string multiple times, it could be a value such as an interest rate percentage, a discount percentage, a tax rate, or a fixed discount amount.

Those values ​​should be relatively short, so the benefit may not be much.

(These are the only answers I can think of right now, there may be other use cases.)

@nielsdos
Copy link
Member Author

nielsdos commented May 2, 2024

Right, would still be nice to have specific numbers.
One thing that might help in the case of money calculations is caching the result too. When a result is then used in a subsequent calculation we can avoid parsing it.
I think that would be quite common because people often use results from one calculation in a further calculation.
What do you think?

@SakiTakamachi
Copy link
Member

One thing that might help in the case of money calculations is caching the result too. When a result is then used in a subsequent calculation we can avoid parsing it.
I think that would be quite common because people often use results from one calculation in a further calculation.

Yes, that use case definitely exists.

Note however that if BCMath were to support object types, it would keep bc_num inside the object, so this cache may be dedicated to existing functions only.

@frederikbosch
Copy link
Contributor

You might want to use the BcMathCalculatorBench included in MoneyPHP.

@nielsdos
Copy link
Member Author

nielsdos commented May 2, 2024

You might want to use the BcMathCalculatorBench included in MoneyPHP.

Thanks for the pointer, this will be useful for our testing!
I'll test that with this PR sometime later.

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.

Code looks logical, will wait for benchmark results

@nielsdos
Copy link
Member Author

nielsdos commented May 4, 2024

Running the moneyphp benchmark showed that this has no benefit, at least for that testcase: I see some small gains and some larger losses.
The problem is that in that benchmark only very small numbers are tested, so the overhead of caching then outweighs the gains we would get. If the benchmark were to use larger numbers then we would see different results. So I'll hold off on this.

@nielsdos
Copy link
Member Author

Closing this as this will not move forward in its current state.

@nielsdos nielsdos closed this May 14, 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.

None yet

4 participants