[go: nahoru, domu]

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

ext/bcmath: Optimize bcdiv processing #14660

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

SakiTakamachi
Copy link
Member
@SakiTakamachi SakiTakamachi commented Jun 25, 2024

Changed to use BC_VECTOR to calculate faster.
Since there is a considerable speed difference, the benchmark was roughly measured.
The division code was changed in the third commit.

benchmark code:

<?php

// 1
$num1 = '1.23';
$num2 = '2';

$start = microtime(true);
for ($i = 0; $i < 500000; $i++) {
    bcdiv($num1, $num2, 0);
}
echo microtime(true) - $start . PHP_EOL;

// 2
$num1 = '4.123456';
$num2 = '0.001';

$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    bcdiv($num1, $num2, 0);
}
echo microtime(true) - $start . PHP_EOL;

// 3
$num1 = '1.23';
$num2 = '2.12';

$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    bcdiv($num1, $num2, 4);
}
echo microtime(true) - $start . PHP_EOL;

// 4
$num1 = '5.2345678';
$num2 = '2.1234567';

$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    bcdiv($num1, $num2, 0);
}
echo microtime(true) - $start . PHP_EOL;

// 5
$num1 = '5.23456789';
$num2 = '2.12345678';

$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    bcdiv($num1, $num2, 0);
}
echo microtime(true) - $start . PHP_EOL;

// 6
$num1 = '1.234567812345678';
$num2 = '2.123456712345678';

$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    bcdiv($num1, $num2, 50);
}
echo microtime(true) - $start . PHP_EOL;

// 7
$num1 = '1.234567812345678';
$num2 = '2.1';

$start = microtime(true);
for ($i = 0; $i < 1000000; $i++) {
    bcdiv($num1, $num2, 50);
}
echo microtime(true) - $start . PHP_EOL;

// 8
$num1 = str_repeat('1234567890', 300);
$num2 = str_repeat('9876543210', 300);

$start = microtime(true);
for ($i = 0; $i < 5000; $i++) {
    bcdiv($num1, $num2, 50);
}
echo microtime(true) - $start . PHP_EOL;

before:

0.13476610183716 // 1
0.28906989097595 // 2
0.29561901092529 // 3
0.26175594329834 // 4
0.25259518623352 // 5
2.5445368289948 // 6
1.1578629016876 // 7
1.598653793335 // 8

after:

0.080783128738403 // 1
0.15704202651978 // 2
0.15926694869995 // 3
0.16794800758362 // 4
0.17711997032166 // 5
0.32082390785217 // 6
0.28861498832703 // 7
0.048795223236084 // 8

@SakiTakamachi
Copy link
Member Author

There was a mistake in some of the assumptions, which made the code unnecessarily complicated, so I will correct it.

@SakiTakamachi
Copy link
Member Author

done

Comment on lines +73 to +79
BC_VECTOR bc_parse_chunk_chars(const char *str)
{
BC_VECTOR tmp;
memcpy(&tmp, str, sizeof(tmp));
#if !BC_LITTLE_ENDIAN
tmp = BC_BSWAP(tmp);
#endif
Copy link
Contributor
@jorgsowa jorgsowa Jun 26, 2024

Choose a reason for hiding this comment

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

If I see correctly this part is the same for both sizes, so it can be excluded from the condition.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is originally Niels' code, and is written this way for readability.

I would like to leave his code untouched.

Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt it's more readable when we repeat the second level if condition in both functions, but it's a subjective opinion. For sure cognitive complexity is lower without nesting if conditions. It's not a big deal though.

@SakiTakamachi
Copy link
Member Author

It could probably be made a bit less complicated.

I'll fix it and rebase it.

@SakiTakamachi SakiTakamachi force-pushed the refactor_bcdiv branch 7 times, most recently from 67b9e10 to e442f59 Compare June 28, 2024 07:53
@SakiTakamachi
Copy link
Member Author

done

BC_VECTOR div_carry = 0;

/*
* If calculate the temporary quotient using only one array of n1 and n2, the error will be greater
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this rule have a name? If so, please let me know, as I can simply provide the URL.

@SakiTakamachi
Copy link
Member Author

@Girgias @nielsdos

This is all the changes in this PR, so I would appreciate it if you could check it out when you have time :)

@nielsdos
Copy link
Member
nielsdos commented Jul 4, 2024

First two commits are definitely fine to already land on master. I'll try to have a look at the actual algorithmic changes today.

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

3 participants