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.

Sign up for GitHub

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

Jump to bottom

Optimise binary arithmetic operators for powers of 10 #23

Merged

faheel merged 8 commits into faheel:master from MagmaCode:powers_of_10

Feb 8, 2018

Contributor

MagmaCode commented Feb 5, 2018

I gave a try to optimising the binary operations with powers of 10. #18
Hope it is good enough. First open source contribution. 👍

MagmaCode added 5 commits

February 5, 2018 12:00


          Added is_power_of_10() to utility.hpp

29c3265


          Added get_power_of_10() to utility.hpp

128be00


          Optimised division for powers of 10

84304c3


          Optimised multiplication for powers of 10

1b3686b


          Optimised modulo operations for powers of 10

17f226c

codecov bot commented Feb 5, 2018 •

edited

Loading

Codecov Report

Merging #23 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #23   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines         517    536   +19     
=====================================
+ Hits          517    536   +19

Impacted Files	Coverage Δ
include/operators/binary_arithmetic.hpp	`100% <100%> (ø)`	⬆️
include/functions/utility.hpp	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00f3910...62e2e67. Read the comment docs.

faheel requested changes

View reviewed changes

Owner

faheel left a comment

Thank you for optimising BigInt! Following are some suggestions to make it even better:

include/functions/utility.hpp Outdated

+                  Checks whether a string-represented integer is a power of 10.
+              */
+              bool is_power_of_10(std::string num){

Owner

faheel Feb 6, 2018

Accept string argument as a constant reference (const std::string& num).

include/functions/utility.hpp Outdated

+                  if(num == "1" or num == "10") return 1;
+                  if (num.front() != '1') return 0;
+                  while( num.back() == '0'){
+                      num.pop_back();

Owner

faheel Feb 6, 2018

No need to modify the string by popping characters. First check that the first digit is '1', then just traverse over the other digits, and if a digit is not found to be '0', return false. At the end (outside the loop) return true.

include/functions/utility.hpp Outdated

+                  Checks whether a string-represented integer is a power of 10.
+              */
+              size_t get_power_of_10(std::string num){

Owner

faheel Feb 6, 2018

This function is not needed. See the following comments for alternatives.

include/operators/binary_arithmetic.hpp Outdated

@@ @@ -156,6 +156,14 @@ BigInt BigInt::operator*(const BigInt& num) const { @@
                   BigInt product;
                   if (abs(*this) <= FLOOR_SQRT_LLONG_MAX and abs(num) <= FLOOR_SQRT_LLONG_MAX)
                       product = std::stoll(this->value) * std::stoll(num.value);
+                  else if (is_power_of_10(this->value)){ // if LHS is a power of 10 do optimised operation
+                      product = num;
+                      add_trailing_zeroes(product.value, get_power_of_10(this->value));

Owner

faheel Feb 6, 2018

Instead of adding trailing zeroes equal to the power of 10, use the following (it's much more efficient):

product.value.append(this->value.begin() + 1, this->value.end());

include/operators/binary_arithmetic.hpp Outdated

@@ @@ -156,6 +156,14 @@ BigInt BigInt::operator*(const BigInt& num) const { @@
                   BigInt product;
                   if (abs(*this) <= FLOOR_SQRT_LLONG_MAX and abs(num) <= FLOOR_SQRT_LLONG_MAX)
                       product = std::stoll(this->value) * std::stoll(num.value);
+                  else if (is_power_of_10(this->value)){ // if LHS is a power of 10 do optimised operation
+                      product = num;

Owner

faheel Feb 6, 2018

The following will be much faster:

product.value = num.value;

(product.sign is taken care of before returning).

include/operators/binary_arithmetic.hpp Outdated

@@ @@ -314,6 +325,9 @@ BigInt BigInt::operator%(const BigInt& num) const { @@
                       remainder = std::stoll(abs_dividend.value) % std::stoll(abs_divisor.value);
                   else if (abs_dividend < abs_divisor)
                       remainder = abs_dividend;
+                  else if (is_power_of_10(num.value)){ // if num is a power of 10 use optimised calculation
+                      remainder = abs_dividend.value.substr(abs_dividend.value.size() - get_power_of_10(num.value));

Owner

faheel Feb 6, 2018

Assign to remainder.value instead of remainder.

Owner

faheel Feb 6, 2018

Also, as done for division, no need to use get_power_of_10() here.

include/operators/binary_arithmetic.hpp Outdated

+                  }
+                  else if (is_power_of_10(num.value)){
+                      product = *this;
+                      add_trailing_zeroes(product.value, get_power_of_10(num.value));

Owner

faheel Feb 6, 2018

Similar changes need to be made here as mentioned above.

include/operators/binary_arithmetic.hpp Outdated

@@ @@ -253,6 +261,9 @@ BigInt BigInt::operator/(const BigInt& num) const { @@
                       quotient = std::stoll(abs_dividend.value) / std::stoll(abs_divisor.value);
                   else if (abs_dividend == abs_divisor)
                       quotient = 1;
+                  else if (is_power_of_10(abs_divisor.value)) { // if divisor is a power of 10 do optimised calculation
+                      quotient.value = abs_dividend.value.substr(0, abs_dividend.value.size() - get_power_of_10(abs_divisor.value));

Owner

faheel Feb 6, 2018

No need of using get_power_of_10() here. Instead, you can do:

size_t digits_in_quotient = abs_dividend.value.size() - abs_divisor.value.size() + 1;
quotient.value = abs_dividend.value.substr(0, digits_in_quotient);

faheel changed the title ~~Optimised Powers of 10.~~ Optimise binary arithmetic operators for powers of 10

faheel added enhancement operators: binary arithmetic labels


          Implemented the suggested changes from PR faheel#23

c2dbc95

Contributor Author

MagmaCode commented Feb 6, 2018

I made the changes

Contributor Author

MagmaCode commented Feb 6, 2018 •

edited

Loading

Travis CI seems to have failed from network problems on their part. So I guess it's all good... fixed

MagmaCode closed this

MagmaCode reopened this


          Added test case for when RHS of multiplication is a power of 10

088c33f

faheel requested changes

View reviewed changes

include/functions/utility.hpp Outdated

+                  if (num == "1") return true;
+                  if (num.front() == '1'){
+                      for (char c : num.substr(1)){
+                          if (c != '0') return false;

Owner

faheel Feb 7, 2018

The following is much simpler and more efficient:

if (num[0] != '1')
    return false;
for (size_t i = 1; i < num.size(); i++)
    if (num[i] != '0')
        return false;

return true;    // first digit is 1 and the following digits are all 0

Here's why:

it has less branching
has less number of return statements
doesn't create a substring, which means it doesn't take up extra memory or the extra time to create the substring

test/operators/binary_arithmetic.test.cpp

+                  num1 = big_pow10(19876) + 1;
+                  num2 = big_pow10(23450);
+                  REQUIRE(num1 * num2 == big_pow10(43326) + big_pow10(23450));

Owner

faheel Feb 7, 2018

This test seems confusing at first glance. We can have tests like this when + or - operators are optimised. For now, you may remove it.

Contributor Author

MagmaCode Feb 7, 2018

So should I leave it without a test for this case?

Owner

faheel Feb 7, 2018

You can remove these 4 lines. The code coverage won't drop as tests with powers of 10 already exist.

Contributor Author

MagmaCode Feb 7, 2018 •

edited

Loading

Coverage does drop without the last commit.
There are tests multiplying powers of 10 but then it uses only the LHS(with *this ). So the RHS(with num ) is unused and coverage drops to 99.1%.

Owner

faheel Feb 7, 2018

You're right. Let it be then.


          Made is_power_of_10() better

62e2e67

Contributor Author

MagmaCode commented Feb 7, 2018

All done.

faheel approved these changes

View reviewed changes

faheel merged commit 7bc46cd into faheel:master

Owner

faheel commented Feb 8, 2018

@MagmaCode Thank you for contributing, and congratulations on your first open source contribution! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement operators: binary arithmetic