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

Remove explicit direct cache interactions #366

Merged
merged 1 commit into from
Oct 31, 2018

Conversation

mattdean-digicatapult
Copy link
Contributor

This is part of the broader discussion in #268 and #309 so readers should probably start there.

One of the goals of defining and simplifying the stateManager interface is to allow others to implement their own underlying storage infrastructure which may or may not include caching. The current implementation exposes the cache both through direct accesses by the vm and through the warmCache method on stateManager. The existence of the cache is also exposed by the populateCache option on a couple of the vm methods. This PR acts to remove these in order to allow us to in future hide the existence of the cache from the vm entirely.

The change essentially removes the options, and modifies synchronous cache accesses, which assume the account has been preloaded, to instead operate against stateManager asynchronously. The diff here is a little larger than I would like but a lot of this is down to making accesses asynchronous.

@coveralls
Copy link

coveralls commented Oct 8, 2018

Coverage Status

Coverage decreased (-0.4%) to 91.242% when pulling f9df024 on remove-explicit-direct-cache-interactions into 906eb6c on master.

@holgerd77
Copy link
Member

Hi @mattdean-digicatapult, super-great, thanks for this new PR! I am a bit scratching my head how we get this coordinated on merging and rebasing together with the other also broadly scoped Constantinople PRs, especially the broad changes in the SSTORE PR #367.

If you have ideas on that let me know. //cc @vpulim

@mattdean-digicatapult
Copy link
Contributor Author

Hi @holgerd77, I'm not too worried about either #367 or #329 which I'm thinking would be the two PRs of immediate concern. They both access stateManager "correctly" rather than doing direct cache reads so it should merge ok 🤞.

If those changes are near completion I'm happy to rebase on top of them once they are merged; I get that the Constantinople changes are probably of immediate priority. Alternatively if they are still a way off, and these changes are acceptable, I'm happy to help with the merge of this stuff into those branches to keep things moving.

@holgerd77
Copy link
Member

Hi @mattdean-digicatapult, sounds great, thanks! Yes, you are right, Constantinople stuff has very much high priority atm so we'll likely then first do the Constantinople merges, but would like to come to your work shortly after since this is also highly anticipated.

@holgerd77
Copy link
Member

Since Constantinople is delayed anyhow I think we can in parallel going more actively forward with the StateManager changes, so if you want you can rebase and I'll do another review and we can eventually merge.

@holgerd77
Copy link
Member

holgerd77 commented Oct 31, 2018

(rebasing would be especially helpful since now blockchain tests are running by default - see #341 - and we'll get a lot better test coverage (in the sense: seeing what could be wrong) with this now.)

The method `warmCache` on `stateManager` and the `populateCache` option directly expose
the existence of the state cache. This makes removing all references to the cache impossible.
@mattdean-digicatapult mattdean-digicatapult force-pushed the remove-explicit-direct-cache-interactions branch from 79198f0 to f9df024 Compare October 31, 2018 08:32
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Looks good, just doing the merge together with @mattdean-digicatapult at Devcon having some extra explanations, great work, thanks!

@holgerd77 holgerd77 merged commit 4585cac into master Oct 31, 2018
@holgerd77 holgerd77 deleted the remove-explicit-direct-cache-interactions branch October 31, 2018 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants