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.

Sign up for GitHub

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

Jump to bottom

Improve getAttestationDeltas using Number #2092

Merged
merged 8 commits into from
Mar 2, 2021
Merged

Improve getAttestationDeltas using Number #2092

merged 8 commits into from
Mar 2, 2021

Conversation

twoeths
Copy link
Contributor

@twoeths twoeths commented Feb 23, 2021 •
edited

part of #2046 #2049

  • use Number instead of BigInt whenever possible to improve getAttestationDeltas
  • Average time to run getAttestationDeltas was reduced from 130 to 79

@codeclimate
Copy link

codeclimate bot commented Feb 23, 2021 •
edited

Code Climate has analyzed commit 70d9295 and detected 0 issues on this pull request.

View more on Code Climate.

@lgtm-com
Copy link

lgtm-com bot commented Feb 23, 2021

This pull request introduces 1 alert when merging c135a80 into d3fadca - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@twoeths
Copy link
Contributor Author

twoeths commented Feb 24, 2021

not able to pass spec tests using Number. Also getAttestationDeltas spec tests was so strict, currently we do sum+= c1 * a / c2; sum += c1 * b / c;, if I do sum += c1 * (a+b) / c2; it does not pass spec tests, I guess its due to BigInt calculation

@twoeths twoeths closed this Feb 24, 2021
@twoeths twoeths reopened this Feb 24, 2021
@dapplion
Copy link
Contributor

not able to pass spec tests using Number. Also getAttestationDeltas spec tests was so strict, currently we do sum+= c1 * a / c2; sum += c1 * b / c;, if I do sum += c1 * (a+b) / c2; it does not pass spec tests, I guess its due to BigInt calculation

What's the specific test breaking and the error message?

@lgtm-com
Copy link

lgtm-com bot commented Feb 24, 2021

This pull request introduces 1 alert when merging 3a280da into 8066c40 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@twoeths
Copy link
Contributor Author

twoeths commented Feb 24, 2021

not able to pass spec tests using Number. Also getAttestationDeltas spec tests was so strict, currently we do sum+= c1 * a / c2; sum += c1 * b / c;, if I do sum += c1 * (a+b) / c2; it does not pass spec tests, I guess its due to BigInt calculation

What's the specific test breaking and the error message?

nvm, it works now. Thanks @wemeetagain for the great suggestion of using intDiv

@twoeths twoeths marked this pull request as ready for review February 24, 2021 08:13
wemeetagain
wemeetagain reviewed Feb 25, 2021
View reviewed changes
packages/lodestar-beacon-state-transition/src/phase0/fast/epoch/processRewardsAndPenalties.ts Outdated
@@ -18,14 +18,14 @@ export function processRewardsAndPenalties(
const newBalances = readOnlyMap(state.balances, (balance) => balance);
Copy link
Member

Choose a reason for hiding this comment

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

As an additional optimization, you might try to apply the rewards and penalties at the same time.
eg:

const newBalances = readOnlyMap(state.balances, (balance, i) => {
  const newBalance = balance + BigInt(rewards[i] - penalties[i]);
  return newBalance < 0 ? BigInt(0) : newBalance;
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wemeetagain this helps reduce the time in processRewardsAndPenalties.test.ts ~5%

@twoeths twoeths force-pushed the tuyen/improve-getAttestationDeltas branch from 439ecbc to dcc3574 Compare March 2, 2021 12:34
@lgtm-com
Copy link

lgtm-com bot commented Mar 2, 2021

This pull request introduces 1 alert when merging dcc3574 into 423124c - view on LGTM.com

new alerts:

  • 1 for Useless assignment to property

@twoeths twoeths force-pushed the tuyen/improve-getAttestationDeltas branch from dcc3574 to 0a70311 Compare March 2, 2021 13:00
@lgtm-com
Copy link

lgtm-com bot commented Mar 2, 2021

This pull request introduces 1 alert when merging 0a70311 into 423124c - view on LGTM.com

new alerts:

  • 1 for Useless assignment to property

twoeths
twoeths commented Mar 2, 2021
View reviewed changes
packages/lodestar-beacon-state-transition/test/perf/phase0/fast/slot/slots.test.ts
@@ -27,7 +27,7 @@ describe("Process Slots Performance Test", function () {
const start = Date.now();
phase0.fast.processSlots(epochCtx, state, state.slot + numSlot);
logger.profile(`Process ${numSlot} slots`);
expect(Date.now() - start).lt(1100);
expect(Date.now() - start).lt(570);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

with new ssz + this PR, the new epoch transition takes 500ms - 600ms in average instead of ~1100ms as previously

@wemeetagain wemeetagain merged commit f5f6403 into master Mar 2, 2021
@wemeetagain wemeetagain deleted the tuyen/improve-getAttestationDeltas branch March 2, 2021 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wemeetagain wemeetagain wemeetagain approved these changes

@dapplion dapplion dapplion approved these changes

@mpetrunic mpetrunic Awaiting requested review from mpetrunic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@twoeths @dapplion @wemeetagain