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

WIP: Remove active and shuffle indices caches #3631

Closed
wants to merge 28 commits into from
Closed

WIP: Remove active and shuffle indices caches #3631

wants to merge 28 commits into from

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Sep 28, 2019 •
edited by rauljordan
Loading

Review and merge #3629 first. Resolves #3106.

This PR removes active and shuffle indices caches, it's a nice change because it allows us to remove all the helpers.ClearAllCaches() instances. This will enable us to use the new cache with a feature flag --new-cache

Change list:

  • Removed active indices cache
  • Removed shuffle indices cache
  • Removed active count cache
  • Removed all the instances for helpers.ClearAllCaches(), it's no longer needed
  • Fixed all the tests
  • Tested runtime on a single node + 512 validators
  • Tested run time on a single node + 512 validators + a syncing node

@terencechain terencechain added the Ready For Review A pull request ready for code review label Sep 28, 2019
@terencechain terencechain self-assigned this Sep 28, 2019
@codecov
Copy link

codecov bot commented Sep 29, 2019 •
edited
Loading

Codecov Report

Merging #3631 into master will increase coverage by 10.56%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master    #3631       +/-   ##
===========================================
+ Coverage   48.86%   59.42%   +10.56%     
===========================================
  Files         162      169        +7     
  Lines       10880    11112      +232     
===========================================
+ Hits         5316     6603     +1287     
+ Misses       4687     3640     -1047     
+ Partials      877      869        -8

@terencechain terencechain mentioned this pull request Sep 29, 2019
Closed
8 tasks
shayzluf
shayzluf reviewed Sep 30, 2019
View reviewed changes
beacon-chain/core/helpers/committee_test.go
@@ -175,51 +175,6 @@ func TestComputeCommittee_WithoutCache(t *testing.T) {
}
}

func TestComputeCommittee_WithCache(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't you want to add the same tests when you add in the new cache? maybe just comment ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already tested in in the cache package, since now we are using a feature flag, this was just testing that the feature flag should work

@terencechain @shayzluf
Update beacon-chain/core/epoch/epoch_processing.go
9cd5fc0 
Co-Authored-By: shayzluf <thezluf@gmail.com>
@stale stale bot added the Stale There hasn't been any activity here in some time... label Oct 9, 2019
@rauljordan rauljordan removed the Stale There hasn't been any activity here in some time... label Oct 12, 2019
@stale
Copy link

stale bot commented Oct 19, 2019

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

@stale stale bot added the Stale There hasn't been any activity here in some time... label Oct 19, 2019
@rauljordan rauljordan removed the Stale There hasn't been any activity here in some time... label Oct 21, 2019
@stale
Copy link

stale bot commented Oct 28, 2019

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

@stale stale bot added the Stale There hasn't been any activity here in some time... label Oct 28, 2019
@terencechain terencechain removed the Stale There hasn't been any activity here in some time... label Oct 30, 2019
@rauljordan
Copy link
Contributor

Still relevant @terencechain ?

@terencechain
Copy link
Member Author

@rauljordan yes this should get merged after we use new cache by default

@nisdas nisdas added the Blocked Blocked by research or external factors label Nov 5, 2019
@stale
Copy link

stale bot commented Nov 11, 2019

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

@stale stale bot added the Stale There hasn't been any activity here in some time... label Nov 11, 2019
@rauljordan rauljordan removed the Stale There hasn't been any activity here in some time... label Nov 11, 2019
@stale
Copy link

stale bot commented Nov 18, 2019

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

@stale stale bot added the Stale There hasn't been any activity here in some time... label Nov 18, 2019
@stale
Copy link

stale bot commented Nov 25, 2019

This pull request has been closed due to inactivity. Please reopen this pull request if you would like to continue working on it.

@stale stale bot closed this Nov 25, 2019
@terencechain terencechain reopened this Nov 25, 2019
@terencechain terencechain removed the Stale There hasn't been any activity here in some time... label Dec 2, 2019
@stale
Copy link

stale bot commented Dec 2, 2019

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

@stale stale bot added the Stale There hasn't been any activity here in some time... label Dec 2, 2019
@terencechain terencechain removed the Stale There hasn't been any activity here in some time... label Dec 7, 2019
@stale
Copy link

stale bot commented Dec 9, 2019

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

@stale stale bot added the Stale There hasn't been any activity here in some time... label Dec 9, 2019
@terencechain terencechain mentioned this pull request Dec 11, 2019
Closed
@prestonvanloon prestonvanloon mentioned this pull request Dec 11, 2019
Merged
@0xKiwi
Copy link
Contributor

0xKiwi commented Dec 12, 2019

Closing as this PR has been replaced by #4266.

@0xKiwi 0xKiwi closed this Dec 12, 2019
@terencechain terencechain deleted the improve-cache-8 branch January 6, 2020 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shayzluf shayzluf shayzluf left review comments

@rauljordan rauljordan rauljordan left review comments

Assignees

@terencechain terencechain

Labels
Blocked Blocked by research or external factors Ready For Review A pull request ready for code review Stale There hasn't been any activity here in some time...
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Tracking] Caches are causing unstable results
5 participants
@terencechain @rauljordan @0xKiwi @shayzluf @nisdas