refactor: use ordered publisher list #402
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ali-bahjati
force-pushed
the
refactor/use-ordered-publisher-set
branch
from
5335952 to
8b2f52f
Compare
Reisen
reviewed
May 6, 2024
ali-bahjati
force-pushed
the
refactor/use-ordered-publisher-set
branch
from
8b2f52f to
4fa26a9
Compare
jayantk
reviewed
May 6, 2024
guibescos
reviewed
May 6, 2024
ali-bahjati
force-pushed
the
refactor/use-ordered-publisher-set
branch
2 times, most recently
from
0e81986 to
58afc9d
Compare
jayantk
previously approved these changes
May 9, 2024
ali-bahjati
force-pushed
the
refactor/use-ordered-publisher-set
branch
2 times, most recently
from
6a58b0a to
13aeaef
Compare
This change brings down the average cost of a price update without aggregation from 5k to 2k because binary search requires much less lookups. There are mechanims implemented in the code to make sure that upon the upgrade we sort the array without adding overhead to the happy path (working binary search). Also add_publisher now does an insertion sort to keep the list sorted and del_publisher won't change the order of the list.
ali-bahjati
force-pushed
the
refactor/use-ordered-publisher-set
branch
from
13aeaef to
c27465d
Compare
jayantk
previously approved these changes
May 13, 2024
| { | ||
| price_data.comp_.swap(current_index, current_index - 1); | ||
| current_index -= 1; | ||
| } |
There was a problem hiding this comment.
minor comment: imo, we should call sort_price_comps here as well, because the insertion code above isn't tested by the quickcheck stuff below. We don't add publishers a lot so we don't care about optimizing CU use.
There was a problem hiding this comment.
(feel free to ignore this. I'm only saying it because it feels like the maximally defensive implementation)
There was a problem hiding this comment.
did that. removed the inline hint to make sure code size doesn't get larger (by sacrificing a bit of CU)
ali-bahjati
force-pushed
the
refactor/use-ordered-publisher-set
branch
from
45002cf to
37e99b8
Compare
jayantk
previously approved these changes
May 13, 2024
Reisen
previously approved these changes
May 13, 2024
guibescos
reviewed
May 13, 2024
guibescos
reviewed
May 13, 2024
guibescos
reviewed
May 13, 2024
guibescos
reviewed
May 13, 2024
guibescos
previously approved these changes
May 13, 2024
ali-bahjati
force-pushed
the
refactor/use-ordered-publisher-set
branch
from
46cda78 to
b991cba
Compare
guibescos
approved these changes
May 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change brings down the average cost of a price update without aggregation from 5k to 2k by moving from unordered list of publishers to an ordered list of publishers on doing binary search on lookup. It will fall back to linear search if the binary search fails.
Also add_publisher now tries to sort the list with [0,0,0...] (Base58: 11111...) publisher as a migration path. The sort is implemented in a way to be efficient and small (in code size). There is an insertion sort upon each new publisher to make sure the list remains sorted. del_publisher won't change the order of the list so it will remain sorted.
The heavy code path for publishers now uses
sol_memcmp(on sorting and lookup) to be more optimized (it costs 10 CU and apparently publisher equality is a few times more expensive). I checked sol_memcmp without change to sorted but the result was not as significant. Maybe later we can extract it out to a function, and use it everywhere.