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

multi: optimize MaxOrder #1684

Merged
merged 4 commits into from
Jul 3, 2022
Merged

multi: optimize MaxOrder #1684

merged 4 commits into from
Jul 3, 2022

Conversation

buck54321
Copy link
Member

Resolves #1612

@buck54321 buck54321 force-pushed the maxorder-bugs branch 3 times, most recently from 2d1bfd7 to 07b93e7 Compare June 24, 2022 11:38
Comment on lines +1794 to 1766
// Check if the largest output is too small.
lastUTXO := okUTXOs[len(okUTXOs)-1]
if !isEnoughWith(lastUTXO) {
addUTXO(lastUTXO)
okUTXOs = okUTXOs[0 : len(okUTXOs)-1]
continue
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Moving this check up up here was the critical improvement. Before, we iterated the utxos from smallest to largest to find the smallest utxo that satisfied our needs. If none worked, we'd add the last element and try again. But for MaxOrder, we're using all the utxos, so we'd run the enough function N + (N - 1) + (N - 2) + ... + 1 = N * (N + 1) / 2, or roughly N²/2. With the new pattern, it's just N.

@chappjc
Copy link
Member

chappjc commented Jun 24, 2022

These are smart changes, but I don't think it quite resolves #1612, at least in my case. Initially I thought it must be all this UTXO selection code you're fixing up, but it turned out to be a combination of:

To see what I mean for the first bullet you really need a testnet wallet with hundreds of thousands of utxos.

@buck54321
Copy link
Member Author

buck54321 commented Jun 24, 2022

These are smart changes, but I don't think it quite resolves #1612, at least in my case. Initially I thought it must be all this UTXO selection code you're fixing up, but it turned out to be a combination of:

To see what I mean for the first bullet you really need a testnet wallet with hundreds of thousands of utxos.

Try it out to be sure. I tested with 8000 utxos from the decred simnet wallet, and it shortened the times by a few orders of magnitude. Do you know how many utxos you had? Oh. hundreds of thousands.

re frontend spamming: I've observed that too, but was unable to reproduce after the fix. I can test some more.

@chappjc
Copy link
Member

chappjc commented Jun 24, 2022

Yeah it was definitely sitting on listunspent in that issue, although maxSell, not maxBuy.

Regarding the frontend metering, I think it would ideally be the kind of request that gets fired after nothing has been typed for say 250 ms, similar to many live search boxes on the internet. Instead, how it behaves is if you want to type out 0.001125 in the rate, each digit pressed (after it becomes a valid non-zero rate) immediately sends the maxsell request, and then it'll be metering by the time you get to "5".

@buck54321 buck54321 force-pushed the maxorder-bugs branch 3 times, most recently from 8da6f2d to 220494d Compare June 24, 2022 15:34
@chappjc
Copy link
Member

chappjc commented Jun 24, 2022

That last push solved the maxSell hammering for me.

The maxbuy backend times are also still good.

@buck54321

This comment was marked as outdated.

@chappjc
Copy link
Member

chappjc commented Jun 24, 2022

Perfection! take 2 makes me sooo much less confused about those things.

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Test well. Leaving up for a bit for other reviews.

@@ -884,8 +885,11 @@ export default class MarketsPage extends BasePage {
this.setMaxOrder(mkt.maxSell.swap)
return
}
if (mkt.maxSellRequested) return
Copy link
Member

Choose a reason for hiding this comment

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

This is the critical bit that really resolves #1612 for me

client/asset/dcr/dcr.go Outdated Show resolved Hide resolved
@chappjc chappjc changed the title mutli: optimize MaxOrder multi: optimize MaxOrder Jun 27, 2022
@chappjc chappjc merged commit add7978 into decred:master Jul 3, 2022
@chappjc chappjc added this to the 0.5 milestone Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

max buy/sell endpoints/methods still use too much CPU
3 participants