Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Preserve cache on reverting the snapshot #2488

Merged
merged 3 commits into from
Oct 6, 2016
Merged

Preserve cache on reverting the snapshot #2488

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b60d68
Preserve cache on reverting the snapshot
arkpar Oct 6, 2016
7855737
Renamed merge_with into replace_with
arkpar Oct 6, 2016
5a8bd93
Renamed and documented snapshotting methods
arkpar Oct 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions ethcore/src/state/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,20 @@ impl AccountEntry {
state: AccountState::Clean,
}
}

// Replace data with another entry but preserve storage cache
fn merge_snapshot(&mut self, other: AccountEntry) {
self.state = other.state;
Copy link
Contributor

Choose a reason for hiding this comment

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

what if the new storage is dirty but the other account is clean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Its the other way round. other contains "new" changes that replace self. Everything in self is discarded except for the storage cache

Copy link
Contributor

@gavofyork gavofyork Oct 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

maybe rename other to newer, or override.

Copy link
Contributor

Choose a reason for hiding this comment

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

also it's not really merge - it's more like overlay or replace.

Copy link
Contributor

@rphmeier rphmeier Oct 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Aren't the items in the cache dirtier than those in the snapshots? In revert_snapshot it merges the snapshotted account into the cached, overwriting the cached account with the (cleaner) snapshotted account while preserving the dirty storage (which revert_snapshot should be throwing away?)

The name is a bit confusing too, it seems like it would be used in clear_snapshot but is in fact only used when reverting. (edit: looks like Gav got to this point first)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rphmeier That's correct. Dirty storage is discarded, but the clean storage cache is preserved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed into replace_with

Copy link
Contributor

Choose a reason for hiding this comment

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

just to clarify, the purpose is basically to completely replace the cached account with the snapshotted block, while also preserving any new cached storage values since the snapshotted version was put away?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

match other.account {
None => self.account = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

more readable to keep Some and None in same order, at least locally?

Some(acc) => match self.account {
Some(ref mut ours) => {
ours.merge_with(acc);
},
None => {},
},
}
}
}

/// Representation of the entire state of all accounts in the system.
Expand Down Expand Up @@ -210,8 +224,12 @@ impl State {
let last = self.snapshots.borrow_mut().pop();
if let Some(mut snapshot) = last {
if let Some(ref mut prev) = self.snapshots.borrow_mut().last_mut() {
for (k, v) in snapshot.drain() {
prev.entry(k).or_insert(v);
if prev.is_empty() {
**prev = snapshot;
} else {
for (k, v) in snapshot.drain() {
prev.entry(k).or_insert(v);
}
}
}
}
Expand All @@ -223,7 +241,16 @@ impl State {
for (k, v) in snapshot.drain() {
match v {
Some(v) => {
self.cache.borrow_mut().insert(k, v);
match self.cache.borrow_mut().entry(k) {
Entry::Occupied(mut e) => {
// Merge snapshotted changes back into the main account
// storage preserving the cache.
e.get_mut().merge_snapshot(v);
},
Entry::Vacant(e) => {
e.insert(v);
}
}
},
None => {
match self.cache.borrow_mut().entry(k) {
Expand Down