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

Too many closures retained in heap memory #2720

Closed
twoeths opened this issue Jun 18, 2021 · 5 comments
Closed

Too many closures retained in heap memory #2720

twoeths opened this issue Jun 18, 2021 · 5 comments
Assignees
Labels
prio-low This is nice to have. scope-memory Issues to reduce and improve memory usage.

Comments

@twoeths
Copy link
Contributor

twoeths commented Jun 18, 2021

Describe the bug

When I run benchmark for processInactivityUpdates I found that there are too many closures retained
Screen Shot 2021-06-18 at 09 32 20

Most of them are from the hook when we get sub tree
Screen Shot 2021-06-18 at 09 32 56

Expected behavior

  • Since the hook is important to update the parent tree, I'd think if it's possible to create getSubTreeReadonly to avoid the hook? That should work for some use cases.
  • Find and avoid the below pattern to avoid accessing a property inside state too frequently
for (let i = 0; i < process.statuses.length; i++) {
    const status = process.statuses[i];
    if (hasMarkers(status.flags, FLAG_ELIGIBLE_ATTESTER)) {
      if (hasMarkers(status.flags, FLAG_PREV_TARGET_ATTESTER_OR_UNSLASHED)) {
        state.inactivityScores[i] -= Math.min(1, state.inactivityScores[i]);
      } else {
        state.inactivityScores[i] += Number(INACTIVITY_SCORE_BIAS);
      }
      if (!inActivityLeak) {
        state.inactivityScores[i] -= Math.min(Number(INACTIVITY_SCORE_RECOVERY_RATE), state.inactivityScores[i]);
      }
    }
  }

in the example above, we should get state.inactivityScores only 1 time and assign to a separate variable.
How to reproduce
Run processInactivityUpdates benchmark

@stale
Copy link

stale bot commented Aug 22, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the bot:stale label Aug 22, 2021
@dapplion dapplion added the scope-memory Issues to reduce and improve memory usage. label Aug 23, 2021
@stale stale bot removed the bot:stale label Aug 23, 2021
@dapplion
Copy link
Contributor

@tuyennhv Once we merge all the memory related improvements we should do another round of heap snapshots to validate the improvements. Then we can review this issues 👍

@dapplion
Copy link
Contributor

dapplion commented Sep 3, 2021

Bump

@dapplion
Copy link
Contributor

@tuyennhv Would be nice get a heap snapshot of latest version and compare. With recent ssz bump there's should be no more closures for to tree hooks

@dapplion dapplion added the prio-low This is nice to have. label May 12, 2022
@dapplion
Copy link
Contributor

Closing issues for specific incidents on old versions of Lodestar, please re-open if it happens with latest stable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio-low This is nice to have. scope-memory Issues to reduce and improve memory usage.
Projects
None yet
Development

No branches or pull requests

2 participants