ext/bcmath: Simplify `bc_divide()` code #17987

SakiTakamachi · 2025-03-07T07:43:18Z

While writing the code to make the bc_num struct hold the data as BC_VECTOR* instead of a char*, I realized that the code for bc_divide() could be significantly simplified.

Unfortunately, I won't use the structure change because it slows things down, but I'll try to simplify the code.
As you know, the code was too complicated and I didn't know where the bug was, so simplifying the code is a good thing.

benchmarks

When comparing the speed with the master, it is slightly faster.

1.php:

for ($i = 0; $i < 2000000; $i++) {
    bcdiv('1.23', '2', 5);
}

2.php:

for ($i = 0; $i < 2000000; $i++) {
    bcdiv('5.2345678', '2.1234567', 10);
}

3.php:

for ($i = 0; $i < 2000000; $i++) {
    bcdiv('12345678.901234', '1.2345678912', 10);
}

results 1:

Benchmark 1: /php-dev/sapi/cli/php /mount/bc/div/1.php
  Time (mean ± σ):     309.7 ms ±   7.3 ms    [User: 296.4 ms, System: 6.9 ms]
  Range (min … max):   303.8 ms … 329.5 ms    10 runs
 
Benchmark 2: /master/sapi/cli/php /mount/bc/div/1.php
  Time (mean ± σ):     317.2 ms ±  10.1 ms    [User: 304.7 ms, System: 6.3 ms]
  Range (min … max):   309.6 ms … 342.5 ms    10 runs
 
Summary
  '/php-dev/sapi/cli/php /mount/bc/div/1.php' ran
    1.02 ± 0.04 times faster than '/master/sapi/cli/php /mount/bc/div/1.php'

results 2:

Benchmark 1: /php-dev/sapi/cli/php /mount/bc/div/2.php
  Time (mean ± σ):     372.0 ms ±   3.3 ms    [User: 361.4 ms, System: 4.6 ms]
  Range (min … max):   366.6 ms … 377.6 ms    10 runs
 
Benchmark 2: /master/sapi/cli/php /mount/bc/div/2.php
  Time (mean ± σ):     391.9 ms ±   3.9 ms    [User: 379.5 ms, System: 6.3 ms]
  Range (min … max):   387.7 ms … 401.0 ms    10 runs
 
Summary
  '/php-dev/sapi/cli/php /mount/bc/div/2.php' ran
    1.05 ± 0.01 times faster than '/master/sapi/cli/php /mount/bc/div/2.php'

results 3:

Benchmark 1: /php-dev/sapi/cli/php /mount/bc/div/3.php
  Time (mean ± σ):     492.0 ms ±   5.0 ms    [User: 479.6 ms, System: 6.4 ms]
  Range (min … max):   485.6 ms … 503.8 ms    10 runs
 
Benchmark 2: /master/sapi/cli/php /mount/bc/div/3.php
  Time (mean ± σ):     507.0 ms ±   7.3 ms    [User: 495.0 ms, System: 6.1 ms]
  Range (min … max):   497.5 ms … 519.7 ms    10 runs
 
Summary
  '/php-dev/sapi/cli/php /mount/bc/div/3.php' ran
    1.03 ± 0.02 times faster than '/master/sapi/cli/php /mount/bc/div/3.php'

…early return cases

SakiTakamachi · 2025-03-09T15:30:02Z

ext/bcmath/libbcmath/src/div.c

+		bc_free_num(quot);
+		goto quot_zero;


~~I'm not sure if I should leave this as is...~~
Fixed.

nielsdos · 2025-03-10T07:08:49Z

Can you ping when this is actually ready? Since requesting a review there have been various pushes to the PR so it's unclear when this is ready.

SakiTakamachi · 2025-03-10T07:34:46Z

@nielsdos
sorry, I'll check again this evening and ping if everything is ok.

SakiTakamachi · 2025-03-10T15:07:24Z

@nielsdos

Ready for review🙏

nielsdos · 2025-03-12T19:10:57Z

Ok first two commits are obviously fine already, now taking a look at the refactoring itself.

nielsdos

See my comments (so far).
Did you do any advanced testing like differential fuzzing? We had quite a few bugs in bcmath in early 8.4 releases that could've been caught via differential fuzzing

nielsdos · 2025-03-12T19:18:40Z

ext/bcmath/libbcmath/src/convert.h

+		size_t tmp_len = MIN(BC_VECTOR_SIZE - zeros, nlen);
+		for (size_t i = 0; i < tmp_len; i++) {
+			*n_vector += *nend * base;
+			base *= BASE;


Having both base and BASE is confusing. Please rename the base variable to something more meaningful.

Just to check: this multiplication can't overflow because the highest it can ever get is BC_POW_10_LUT[BC_VECTOR_SIZE] right? (i.e. this function essentially pads zeros to the left side)

fixed in 1c9199d

Just to check: this multiplication can't overflow because the highest it can ever get is BC_POW_10_LUT[BC_VECTOR_SIZE] right? (i.e. this function essentially pads zeros to the left side)

Yes, that's right.

nielsdos · 2025-03-12T19:18:49Z

ext/bcmath/libbcmath/src/convert.h

+	if (zeros > 0) {
+		*n_vector = 0;
+		BC_VECTOR base = BC_POW_10_LUT[zeros];
+		size_t tmp_len = MIN(BC_VECTOR_SIZE - zeros, nlen);


Please pick a more meaningful variable name

fixed in 1c9199d

nielsdos · 2025-03-12T20:02:40Z

ext/bcmath/libbcmath/src/div.c

+{
+	char *qptr = (*quot)->n_value;
+	if (quot_size <= quot_scale) {
+		/* int is 0 */


What does this comment mean?

Your simpler code makes this comment unnecessary, but just to be clear:
This meant "the integer part is 0".

ext/bcmath/libbcmath/src/div.c

nielsdos · 2025-03-12T20:11:00Z

ext/bcmath/libbcmath/src/div.c

 	_bc_rm_leading_zeros(*quot);
 	if (bc_is_zero(*quot)) {
 		(*quot)->n_sign = PLUS;
+		(*quot)->n_scale = 0;


Why is this assignment necessary? This doesn't influence any test either?

Indeed, this was a different change from the PR's purpose...

This is not a change to simplify things, but to eliminate unnecessary processing.
Should I split the PR?

Details:
Here, if n_scale is not 0, the value of quot is something like 0.000.

We can safely remove trailing zeros from bc_num here too, because early return returns a copy of BCG(_zero_) regardless of the scale value.

If do not set the n_scale to 0 here, in the case of the Number class, subsequent calculations will require extra calculations to be performed due to the unnecessary 0.
(Either way, the result is the same: it's a matter of speed.)

edit:

This also applies to multiplication and round, so it may be better to separate them into separate PRs.

Okay I understand. Better in separate PR, that's cleaner and easier to keep an overview of why changes are done in which PR 🙂

Co-authored-by: Niels Dossche <[email protected]>

SakiTakamachi · 2025-03-13T02:46:06Z

@nielsdos

Thanks for your review!

Did you do any advanced testing like differential fuzzing? We had quite a few bugs in bcmath in early 8.4 releases that could've been caught via differential fuzzing

I had completely forgotten about fuzzing, so I tested it.

using: #18036
=> I ran it about 4 million times and no problems were detected.

Additionally, using a technique similar to the fuzz above, we ran a test that generated values in PHP and compared the results with those in 8.3 for 4 million loops. All the outputs matched.

nielsdos

I don't see any clear mistakes.

SakiTakamachi added 2 commits March 7, 2025 11:23

replaced "quot_arr_size - 1" with quot_top_index

884e909

moved POW_10_LUT to private.h

19e09bb

github-actions bot added the Extension: bcmath label Mar 7, 2025

SakiTakamachi marked this pull request as ready for review March 7, 2025 08:38

SakiTakamachi requested review from Girgias and nielsdos as code owners March 7, 2025 08:38

SakiTakamachi force-pushed the bcmath/div branch 3 times, most recently from ead66ef to 6f044c5 Compare March 7, 2025 16:33

Refactored divide

d016743

SakiTakamachi force-pushed the bcmath/div branch from 6f044c5 to d016743 Compare March 7, 2025 16:35

Made it a little easier to see and avoid unnecessary zero padding in …

da0916c

…early return cases

SakiTakamachi commented Mar 9, 2025

View reviewed changes

At the end of the process, the code for when quot is 0 is restored.

8ea5dd6

SakiTakamachi force-pushed the bcmath/div branch from 61d632e to 8ea5dd6 Compare March 10, 2025 01:55

typo

8ecf974

nielsdos requested changes Mar 12, 2025

View reviewed changes

SakiTakamachi and others added 2 commits March 13, 2025 11:16

Simpler code suggestions

56a9028

Co-authored-by: Niels Dossche <[email protected]>

Address comments

1c9199d

(*quot)->n_scale = 0; into a separate PR.

5ed3c74

nielsdos approved these changes Mar 13, 2025

View reviewed changes

[skip ci] NEWS

ff28c14

SakiTakamachi merged commit 4e4f172 into php:master Mar 13, 2025
1 check passed

SakiTakamachi deleted the bcmath/div branch March 13, 2025 23:53

ext/bcmath: Simplify bc_divide() code #17987

ext/bcmath: Simplify bc_divide() code #17987

Uh oh!

Conversation

SakiTakamachi commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

benchmarks

Uh oh!

SakiTakamachi Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsdos commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SakiTakamachi commented Mar 10, 2025

Uh oh!

SakiTakamachi commented Mar 10, 2025

Uh oh!

nielsdos commented Mar 12, 2025

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SakiTakamachi commented Mar 13, 2025

Uh oh!

nielsdos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ext/bcmath: Simplify `bc_divide()` code #17987

ext/bcmath: Simplify `bc_divide()` code #17987

SakiTakamachi commented Mar 7, 2025 •

edited

Loading

SakiTakamachi Mar 9, 2025 •

edited

Loading

nielsdos commented Mar 10, 2025 •

edited

Loading

SakiTakamachi Mar 13, 2025 •

edited

Loading