BREAKING Overlay transaction support. #3263

cheme · 2019-07-30T17:48:02Z

BREAKING: this pr adds externalities, and change mutability of existing one, the corresponding

This PR is a refactor of overlay_change to allow transactional support, it is similar to paritytech/polkadot-sdk#370 goals, but does not stack Ext, it only stacks storage.

It also switches from a stack of hashmap (previously prospective and top) to a single hashmap (containing history of values) and a transaction global state. Those values with state are managed
with 'historied-data' crate (simple vec of data to query in front of a reference global state).

Under this design access to data is not badly impacted by the number of open transactional layers.
I did fuzz this code a bit against a partial simple layered hashmap implementation.

Usage from a runtime with a function, in a similar way as ext_try from paritytech/polkadot-sdk#370, there is the very simple with_transaction function: internally it uses three host functions ext_start_transaction, ext_commit_transaction and ext_discard_transaction. This does not look as good as the single ext_try but is clearer: memory mgmt seems way simplier (as there is none).

Note that to call global state action, modification of local values need to be synchronize.
eg discard_transaction on states must be follow by apply_discard_transaction for all values related to this state and then follow by ensure_running.

Technically we only maintain a counter of current number of stacked transaction as a global state (this start at 1 and can be 0 only to update some values: case of discarding content, but will then return to 1 through 'finalize_discard' call).
Local state is either committed or the number of stacked transaction when the value was changed, this state is stored with the value history.

polkadot companion: paritytech/polkadot#999

initial values.

pepyakin

This looks like a half way through to #2980! (With another part being an runtime instance state/linear memory concerns). But even with that, I think it might be super useful!

We even might be able to construct ext_try from primitives introduced in this PR and with a future introduced primitives for cloning a wasm instance!

core/executor/src/wasm_executor.rs

core/sr-io/without_std.rs

core/state-machine/src/basic.rs

core/state-machine/src/ext.rs

core/state-machine/Cargo.toml

srml/support/src/storage/mod.rs

core/executor/src/wasm_executor.rs

core/state-machine/Cargo.toml

Remove bench feature, exposing 'set_storage' publicly. Minor fixes.

core/state-machine/src/overlayed_changes.rs

thiolliere · 2019-07-31T08:49:51Z

Currently transactional primitives methods name can be a bit confusing (start_transaction do open an new transactional layer but next to this method there is also submit_transaction that submit an actual chain transaction).
So another name than 'transaction' should be suitable.

Maybe start_transactional_layer

cheme · 2019-07-31T08:51:41Z

Maybe start_transactional_layer

I forgot to update my comment, I change it to 'storage_start_transaction' but maybe 'start_transactional_layer' is better?

Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>

pepyakin · 2019-07-31T13:16:40Z

I am actually not sure about the wording "layer". I.e. transactional layer of what? What if we get another kind of layer? On the other hand, "storage transaction" is unambigious.

core/state-machine/src/changes_trie/build.rs

core/state-machine/src/overlayed_changes.rs

…ay_change

pepyakin

First brief look.

pepyakin · 2020-01-24T11:13:18Z

primitives/io/src/lib.rs

@@ -313,6 +313,25 @@ pub trait Storage {
 	}
 }

+/// Interface for managing transaction within the runtime.
+#[runtime_interface]
+pub trait Transaction {


I wonder if StorageTransactions will be a better name?

pepyakin · 2020-01-24T11:16:41Z

primitives/state-machine/src/transaction_layers.rs

+// You should have received a copy of the GNU General Public License
+// along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
+
+//! Types and method for managing a stack of transactional values.


Suggested change

//! Types and method for managing a stack of transactional values.

//! Types and methods for managing a stack of transactional values.

pepyakin · 2020-01-30T12:16:58Z

primitives/state-machine/src/ext.rs

+				(_, Some(overlay_key)) => if overlay_key.1.value.is_some() {
+					return Some(overlay_key.0.to_vec())
+				} else {
+					// TODO make this function non recursive to avoid this clone


Does this TODO need to be fixed before landing this? If not, an issue is required.

No those TODO were just a note to myself, I am removing them.

pepyakin · 2020-01-30T12:18:04Z

primitives/state-machine/src/ext.rs

+		};
+		// TODO no need to query child at each recursive iter here
+		// and also find a way to remove the clone (non recursive and global mut handle should do the
+		// trick).


Does this TODO need to be fixed before landing this? If not, an issue is required.

pepyakin · 2020-01-30T12:19:45Z

primitives/state-machine/src/ext.rs

@@ -136,8 +136,7 @@ where

 		self.backend.pairs().iter()
 			.map(|&(ref k, ref v)| (k.to_vec(), Some(v.to_vec())))
-			.chain(self.overlay.committed.top.clone().into_iter().map(|(k, v)| (k, v.value)))
-			.chain(self.overlay.prospective.top.clone().into_iter().map(|(k, v)| (k, v.value)))
+			.chain(self.overlay.changes.iter_values(None).map(|(k, v)| (k.to_vec(), v.map(|s| s.to_vec()))))


The line exceeds the recommended limit for 100 characters. Can we wrap this line?

pepyakin · 2020-01-30T12:38:46Z

primitives/state-machine/src/transaction_layers.rs

+	TxPending,
+	/// The transaction has been discarded.
+	/// Data from a `LayerEntry` pointing to this layer state should
+	/// not be returned and can be remove.


typo: can be removed

pepyakin · 2020-01-30T13:05:12Z

primitives/state-machine/src/transaction_layers.rs

+	/// Get latest prospective value, excludes
+	/// committed values.
+	pub(crate) fn get_prospective(&self, states: &States) -> Option<&V> {
+		let self_len = self.len();


I find it a bit confusing that the definition of len is in another module. Can we move these definitions closer together?

pepyakin · 2020-01-30T13:05:38Z

primitives/state-machine/src/transaction_layers.rs

+/// triggering.
+///
+/// With this default values it should be very unlikelly that gc is needed
+/// during a block processing for most use case.


typo: use cases

pepyakin · 2020-01-30T13:07:45Z

primitives/state-machine/src/transaction_layers.rs

+	// This information is redundant as it could be
+	// calculated by iterating backward over `history`
+	// field.
+	// Managing this cache allow use to have


pepyakin · 2020-01-30T13:12:18Z

primitives/state-machine/src/transaction_layers.rs

+		None
+	}
+
+	/// Push a value without checking without transactional layer


typo: redundant without?

Significant change of the implementation after my look.

cover all case but is a good entry point to read this PR.

cheme · 2020-05-14T13:11:31Z

This is stale, closing it, @athei will be working on the subject, keeping it open will only create confusion.
Still if some want to use it please don't hesitate to contact me (it got nice complexity property).

cheme added 12 commits July 9, 2019 15:05

init some bench

ef20f40

Init implementing.

a218162

Initial implementation of prospective with unbound layer.

4e8f6a7

Remove size counter

917e26b

fix ix.

8b93ab7

code for transactional layers: TODO testing!!

ae8e880

Regain perfs (-15%) by using a couple of static memory for

c716f95

initial values.

Fix tx logic and basic tx test.

808a409

with_transaction function and high level test.

e481952

state was useless in overlayed change (allways the last ix of history).

d9f3e50

Cleaning comments and names.

cc0aa37

Merge branch 'master' into overlay_change

2bc2af0

cheme added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jul 30, 2019

devops-parity added the B2-breaksapi label Jul 30, 2019

pepyakin reviewed Jul 30, 2019

View reviewed changes

Demi-Marie suggested changes Jul 30, 2019

View reviewed changes

cheme added 2 commits July 31, 2019 10:22

Rename Externalities primitives to 'storage_' + old name.

0c8235e

Remove bench feature, exposing 'set_storage' publicly. Minor fixes.

Merge branch 'master' into overlay_change

5b98b4b

thiolliere reviewed Jul 31, 2019

View reviewed changes

core/state-machine/src/overlayed_changes.rs Outdated Show resolved Hide resolved

cheme and others added 2 commits July 31, 2019 14:54

name fields instead of tuble.

cade913

Update srml/support/src/storage/mod.rs

fd26b2a

Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>

Demi-Marie suggested changes Jul 31, 2019

View reviewed changes

cheme added 5 commits August 1, 2019 09:15

Use more efficient smallvec.

3245968

Merge branch 'overlay_change' of github.com:cheme/polkadot into overl…

8550ad9

…ay_change

Missing spaces.

41483b5

slice and slice to_vec from ptr to vec.

44f5173

Init some fuzzing (only look for panic).

00326b2

tomusdrw mentioned this pull request Jan 28, 2020

RPC api for offchain storage #4694

Merged

pepyakin reviewed Jan 30, 2020

View reviewed changes

cheme added 6 commits January 30, 2020 18:28

Fixing line lengths.

b109130

Merge branch 'master' into overlay_change

2533b04

increment spec version

1db0bec

storage transaction host module name.

121e64b

3 new function, switching index of missing one in wasmtime test.

1e2497a

another hardcoded wasmtime index in test.

46e54c8

cheme added the B1-apinoteworthy label Feb 3, 2020

cheme added 5 commits February 5, 2020 16:14

Merge branch 'master' into overlay_change

1e8cd7b

Merge branch 'master' into overlay_change

1667f18

Merge branch 'master' into overlay_change

5df5e51

Merge branch 'master' into overlay_change

0d29a09

Added a descriptive test of internal state. This does not intend to

4d6af74

cover all case but is a good entry point to read this PR.

pepyakin mentioned this pull request Mar 2, 2020

Split ext_clear_storage out from ext_set_storage #5103

Merged

4 tasks

cheme added 3 commits March 10, 2020 14:20

Merge branch 'master' into overlay_change

6242ebc

update fuzzer cargo.lock

683abcb

Merge branch 'master' into overlay_change

0c8d9e7

gnunicorn assigned cheme Apr 14, 2020

Merge branch 'master' into overlay_change

e620a05

cheme requested a review from NikVolf as a code owner April 14, 2020 15:42

fix bench

45d5c7c

cheme mentioned this pull request Apr 14, 2020

Companion PR for 3263 paritytech/polkadot#999

Closed

iorveth mentioned this pull request May 13, 2020

Content dir 2 Joystream/joystream#396

Merged

49 tasks

cheme mentioned this pull request May 14, 2020

Refactor OverlayedChanges #5989

Merged

cheme closed this May 14, 2020

iorveth mentioned this pull request Jun 3, 2020

Long-running PR for v2.0 of the content directory Joystream/joystream#540

Closed

49 tasks

athei mentioned this pull request Jun 5, 2020

Implement nested storage transactions #6269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING Overlay transaction support. #3263

BREAKING Overlay transaction support. #3263

cheme commented Jul 30, 2019 •

edited

pepyakin left a comment

thiolliere commented Jul 31, 2019

cheme commented Jul 31, 2019 •

edited

pepyakin commented Jul 31, 2019 •

edited

pepyakin left a comment

pepyakin Jan 24, 2020

pepyakin Jan 24, 2020

pepyakin Jan 30, 2020

cheme Jan 30, 2020

pepyakin Jan 30, 2020

pepyakin Jan 30, 2020

pepyakin Jan 30, 2020

pepyakin Jan 30, 2020

pepyakin Jan 30, 2020

pepyakin Jan 30, 2020

pepyakin Jan 30, 2020

cheme commented May 14, 2020

	//! Types and method for managing a stack of transactional values.
	//! Types and methods for managing a stack of transactional values.

BREAKING Overlay transaction support. #3263

BREAKING Overlay transaction support. #3263

Conversation

cheme commented Jul 30, 2019 • edited

pepyakin left a comment

Choose a reason for hiding this comment

thiolliere commented Jul 31, 2019

cheme commented Jul 31, 2019 • edited

pepyakin commented Jul 31, 2019 • edited

pepyakin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheme commented May 14, 2020

cheme commented Jul 30, 2019 •

edited

cheme commented Jul 31, 2019 •

edited

pepyakin commented Jul 31, 2019 •

edited