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

Fast best offers improvements #2340

Merged
merged 5 commits into from Nov 17, 2019
Merged

Conversation

@jonjove
Copy link
Contributor

jonjove commented Nov 12, 2019

This PR adds some incremental improvements on top of #2307, which are largely focused on the interaction between stellar-core and the database. Do not merge until #2307 has been merged.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v5.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document
@jonjove jonjove requested a review from MonsieurNicolas Nov 12, 2019
@jonjove jonjove force-pushed the jonjove:fast-best-offers-sql branch 2 times, most recently from 08a7b8d to 25d408c Nov 13, 2019
@MonsieurNicolas MonsieurNicolas added this to In progress in v12.2.0 via automation Nov 14, 2019
return {};
iter = std::upper_bound(
iter, end, worseThan,
(bool (*)(OfferDescriptor const&, LedgerEntry const&))isBetterOffer);

This comment has been minimized.

Copy link
@MonsieurNicolas

MonsieurNicolas Nov 15, 2019

Contributor

nit: use static_cast instead of c-style cast

This comment has been minimized.

Copy link
@jonjove

jonjove Nov 15, 2019

Author Contributor

Done.

while (!res && !cached->allLoaded)
{
size_t const BATCH_SIZE =
std::min(MAX_BATCH_SIZE, std::max<size_t>(1, offers.size()));

This comment has been minimized.

Copy link
@MonsieurNicolas

MonsieurNicolas Nov 15, 2019

Contributor

I am surprised that we'd start by loading 1 offer as I imagine the latency to the database being dominant.
What is driving the value of the starting point?
If we don't really know, would the safe change be to start with the same number than before (so 5?)

This comment has been minimized.

Copy link
@jonjove

jonjove Nov 15, 2019

Author Contributor

This is a good point, I don't really know what a good value for this is. The old value of 5 seems reasonable, so I will change to that.

src/ledger/LedgerTxnOfferSQL.cpp Show resolved Hide resolved
sql += " ORDER BY price, offerid LIMIT :n OFFSET :o";
std::string sql =
"WITH r1 AS (SELECT * FROM offers "
"WHERE sellingasset = :v1 AND buyingasset = :v2 "

This comment has been minimized.

Copy link
@MonsieurNicolas

MonsieurNicolas Nov 15, 2019

Contributor

this seems to be a very complicated query...

can't we just do something like:

                      "WHERE sellingasset = :v1 AND buyingasset = :v2 " AND price >= :wp
                      "ORDER BY price, offerid LIMIT :n OFFSET :o";

where wp binds to worseThanPrice
and o binds to the number of offers in offers with a price equal to worseThanPrice

@jonjove jonjove force-pushed the jonjove:fast-best-offers-sql branch from 3a21136 to 2315194 Nov 15, 2019
@MonsieurNicolas

This comment has been minimized.

Copy link
Contributor

MonsieurNicolas commented Nov 17, 2019

r+ 2315194

@latobarita

This comment has been minimized.

Copy link
Contributor

latobarita commented on 2315194 Nov 17, 2019

saw approval from MonsieurNicolas
at jonjove@2315194

This comment has been minimized.

Copy link
Contributor

latobarita replied Nov 17, 2019

merging jonjove/stellar-core/fast-best-offers-sql = 2315194 into auto

This comment has been minimized.

Copy link
Contributor

latobarita replied Nov 17, 2019

jonjove/stellar-core/fast-best-offers-sql = 2315194 merged ok, testing candidate = 3448f0e

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

latobarita replied Nov 17, 2019

fast-forwarding master to auto = 3448f0e

latobarita added a commit that referenced this pull request Nov 17, 2019
Fast best offers improvements

Reviewed-by: MonsieurNicolas
@latobarita latobarita merged commit 2315194 into stellar:master Nov 17, 2019
1 check passed
1 check passed
default all tests passed
Details
v12.2.0 automation moved this from In progress to Done Nov 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v12.2.0
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.