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

Uint128_t native #3205

Merged
merged 3 commits into from Oct 9, 2021
Merged

Uint128_t native #3205

merged 3 commits into from Oct 9, 2021

Conversation

graydon
Copy link
Contributor

@graydon graydon commented Sep 11, 2021

This is an experiment in using compiler-native uint128_t types when they are available (on gcc and clang), as a possible performance improvement over the library type. Need to discuss a bit, but it seems to work.

@graydon graydon requested review from MonsieurNicolas and jonjove and removed request for MonsieurNicolas September 11, 2021 23:38
@MonsieurNicolas MonsieurNicolas added this to In progress in v18.1.0 Sep 29, 2021
@graydon graydon force-pushed the uint128-t-native branch 2 times, most recently from 338987b to 6cb63c0 Compare October 2, 2021 07:09
@graydon
Copy link
Contributor Author

graydon commented Oct 2, 2021

Updated to fix bug in uint128_bits

Rounding::ROUND_UP);
opsToFlood =
mBroadcastOpCarryover +
bigDivide(opsToFloodLedger, int64_t(cfg.FLOOD_TX_PERIOD_MS),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be safer to just avoid function overloading (as it's way too easy to forget to cast like this), we can just rename the version that takes an uint128 into bigDivide128 or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed at some length -- wound up renaming all the overloads in numeric.h because I couldn't tell in a lot of cases which overload was even being called in which order. Easiest fix was to split them all apart and just check (and possibly rewrite) each callsite to call the version that corresponds to what I think was meant by the operands passed and surrounding code. This should be carefully reviewed, especially by @jonjove !

@MonsieurNicolas
Copy link
Contributor

r+ 177c166

latobarita added a commit that referenced this pull request Oct 8, 2021
Uint128_t native

Reviewed-by: MonsieurNicolas
@MonsieurNicolas
Copy link
Contributor

I had to stop this while getting 18.0.3 out the door, rekicking

@MonsieurNicolas
Copy link
Contributor

@latobarita: retry

@latobarita latobarita merged commit b08d3f6 into stellar:master Oct 9, 2021
v18.1.0 automation moved this from In progress to Done Oct 9, 2021
@graydon graydon deleted the uint128-t-native branch July 4, 2022 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants