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

Number class shows degradation in math operations performance. #4368

Closed
gregtatcam opened this issue Dec 14, 2022 · 2 comments
Closed

Number class shows degradation in math operations performance. #4368

gregtatcam opened this issue Dec 14, 2022 · 2 comments
Assignees
Labels
Perf Test Desired (Optional) RippleX Perf Team should look at this PR. The PR will not necessarily wait for testing to finish Request for Comments

Comments

@gregtatcam
Copy link
Collaborator

Issue Description

Number class shows degradation in math operations performance as compared to similar operations
implemented in STAmount.

Steps to Reproduce

Here is a simple unit test, which demonstrates the issue:

void
testSwap1()
{
    testcase("Swap1");
    using namespace jtx;

    std::uint16_t tfee = 1000;
    auto const gw = Account("gw");
    auto const USD = gw["USD"];
    auto const GBP = gw["GBP"];
    auto const issueIn = USD;
    auto const issueOut = GBP;
    auto const in = STAmount{USD, 1000};
    auto const out = STAmount{GBP, 1000};
    auto const assetIn = STAmount{USD, 1};
    auto const assetOut = STAmount{GBP, 1};

    auto start = std::chrono::high_resolution_clock::now();
    for (int i = 0; i < 100; ++i)
    {
        auto const fm = 1 - Number{tfee} / 100000;
        auto const res = in - (in * out) / (in + assetIn * fm);
    }
    auto elapsed = std::chrono::high_resolution_clock::now() - start;
    std::cout << "Number(swapIn) math: " << std::chrono::duration_cast<std::chrono::microseconds>(
                                        elapsed).count() << std::endl;

    start = std::chrono::high_resolution_clock::now();
    for (int i = 0; i < 100; ++i)
    {
        auto const n1 = STAmount{1};
        auto const feeMult = n1 - divide(STAmount{tfee}, STAmount{100000}, n1.issue());
        auto const en = multiply(in, out, issueOut);
        auto const den = in + multiply(assetIn, feeMult, issueIn);
        auto const res = out - divide(en, den, issueOut);
    }
    elapsed = std::chrono::high_resolution_clock::now() - start;
    std::cout << "STAmount(swapIn) math: " << std::chrono::duration_cast<std::chrono::microseconds>(
        elapsed).count() << std::endl;
    BEAST_EXPECT(true);
}

Expected Result

Math operations using Number and STAmount function should produce similar results.

Actual Result

The issue manifested itself in the payment engine updated for AMM. Swap in formula used
to figure out initial AMM offer size resulted in the throughput degradation.

Environment

The benchmark test ran on 11 "Ubuntu 20.04.4 LTS" servers (five validators, four clients, two signing) in AWS.
The unit test ran on OSX Ventura.
AMM PR was used to build rippled: #4294

@HowardHinnant HowardHinnant self-assigned this Dec 14, 2022
@intelliot intelliot moved this to 🆕 New in Core Ledger Dec 14, 2022
@intelliot intelliot moved this from 🆕 New to 🏗 In progress in Core Ledger Dec 14, 2022
@HowardHinnant
Copy link
Contributor

I believe this commit fully addresses this issue: 16f1057

This commit is part of the NumberMerge PR: #4192

@intelliot
Copy link
Collaborator

Fixed by 16f1057

@intelliot intelliot moved this from 🏗 In progress to 🚢 Released in 1.10 in Core Ledger Apr 10, 2023
@intelliot intelliot added Perf Test Desired (Optional) RippleX Perf Team should look at this PR. The PR will not necessarily wait for testing to finish and removed Performance/Resource Improvement labels Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Perf Test Desired (Optional) RippleX Perf Team should look at this PR. The PR will not necessarily wait for testing to finish Request for Comments
Projects
Archived in project
Development

No branches or pull requests

3 participants