This repository was archived by the owner on Nov 15, 2023. It is now read-only.

BREAKING Overlay transaction support. - #3263

Closed
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change
Closed

BREAKING Overlay transaction support.#3263
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change

Conversation

@cheme

@chemecheme commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

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

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

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/state-machine/src/basic.rs Outdated
Comment threadcore/state-machine/src/ext.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@gui1117

Copy link
Copy Markdown
Contributor

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

cheme commented Jul 31, 2019

Copy link
Copy Markdown
ContributorAuthor

Maybe start_transactional_layer

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

chemeand others added 2 commits July 31, 2019 14:54
Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>
@pepyakin

pepyakin commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

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.

Comment threadcore/state-machine/src/changes_trie/build.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@tomusdrwtomusdrw mentioned this pull request Jan 28, 2020

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First brief look.

Comment threadprimitives/io/src/lib.rs Outdated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if StorageTransactions will be a better name?

// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
(_, 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
};
// 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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
.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()))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: can be removed

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: use cases

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: us?

None
}

/// Push a value without checking without transactional layer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: redundant without?

@pepyakin
pepyakin dismissed their stale reviewJanuary 30, 2020 15:01

Significant change of the implementation after my look.

@cheme
cheme requested a review from NikVolf as a code ownerApril 14, 2020 15:42
@iorvethiorveth mentioned this pull request May 13, 2020
49 tasks
@chemecheme mentioned this pull request May 14, 2020
@cheme

Copy link
Copy Markdown
ContributorAuthor

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).

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_reviewPull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@cheme@gui1117@pepyakin@Demi-Marie@bkchr@gavofyork@svyatonik@shawntabrizi@kianenigma@gnunicorn@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

BREAKING Overlay transaction support. - #3263

Closed
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change
Closed

BREAKING Overlay transaction support.#3263
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change

Conversation

@cheme

@chemecheme commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

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

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

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/state-machine/src/basic.rs Outdated
Comment threadcore/state-machine/src/ext.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@gui1117

Copy link
Copy Markdown
Contributor

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

cheme commented Jul 31, 2019

Copy link
Copy Markdown
ContributorAuthor

Maybe start_transactional_layer

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

chemeand others added 2 commits July 31, 2019 14:54
Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>
@pepyakin

pepyakin commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

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.

Comment threadcore/state-machine/src/changes_trie/build.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@tomusdrwtomusdrw mentioned this pull request Jan 28, 2020

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First brief look.

Comment threadprimitives/io/src/lib.rs Outdated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if StorageTransactions will be a better name?

// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
(_, 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
};
// 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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
.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()))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: can be removed

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: use cases

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: us?

None
}

/// Push a value without checking without transactional layer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: redundant without?

@pepyakin
pepyakin dismissed their stale reviewJanuary 30, 2020 15:01

Significant change of the implementation after my look.

@cheme
cheme requested a review from NikVolf as a code ownerApril 14, 2020 15:42
@iorvethiorveth mentioned this pull request May 13, 2020
49 tasks
@chemecheme mentioned this pull request May 14, 2020
@cheme

Copy link
Copy Markdown
ContributorAuthor

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).

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_reviewPull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@cheme@gui1117@pepyakin@Demi-Marie@bkchr@gavofyork@svyatonik@shawntabrizi@kianenigma@gnunicorn@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

BREAKING Overlay transaction support. - #3263

Closed
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change
Closed

BREAKING Overlay transaction support.#3263
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change

Conversation

@cheme

@chemecheme commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

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

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

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/state-machine/src/basic.rs Outdated
Comment threadcore/state-machine/src/ext.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@gui1117

Copy link
Copy Markdown
Contributor

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

cheme commented Jul 31, 2019

Copy link
Copy Markdown
ContributorAuthor

Maybe start_transactional_layer

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

chemeand others added 2 commits July 31, 2019 14:54
Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>
@pepyakin

pepyakin commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

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.

Comment threadcore/state-machine/src/changes_trie/build.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@tomusdrwtomusdrw mentioned this pull request Jan 28, 2020

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First brief look.

Comment threadprimitives/io/src/lib.rs Outdated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if StorageTransactions will be a better name?

// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
(_, 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
};
// 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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
.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()))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: can be removed

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: use cases

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: us?

None
}

/// Push a value without checking without transactional layer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: redundant without?

@pepyakin
pepyakin dismissed their stale reviewJanuary 30, 2020 15:01

Significant change of the implementation after my look.

@cheme
cheme requested a review from NikVolf as a code ownerApril 14, 2020 15:42
@iorvethiorveth mentioned this pull request May 13, 2020
49 tasks
@chemecheme mentioned this pull request May 14, 2020
@cheme

Copy link
Copy Markdown
ContributorAuthor

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).

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_reviewPull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@cheme@gui1117@pepyakin@Demi-Marie@bkchr@gavofyork@svyatonik@shawntabrizi@kianenigma@gnunicorn@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

BREAKING Overlay transaction support. - #3263

Closed
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change
Closed

BREAKING Overlay transaction support.#3263
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change

Conversation

@cheme

@chemecheme commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

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

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

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/state-machine/src/basic.rs Outdated
Comment threadcore/state-machine/src/ext.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@gui1117

Copy link
Copy Markdown
Contributor

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

cheme commented Jul 31, 2019

Copy link
Copy Markdown
ContributorAuthor

Maybe start_transactional_layer

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

chemeand others added 2 commits July 31, 2019 14:54
Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>
@pepyakin

pepyakin commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

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.

Comment threadcore/state-machine/src/changes_trie/build.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@tomusdrwtomusdrw mentioned this pull request Jan 28, 2020

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First brief look.

Comment threadprimitives/io/src/lib.rs Outdated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if StorageTransactions will be a better name?

// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
(_, 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
};
// 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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
.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()))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: can be removed

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: use cases

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: us?

None
}

/// Push a value without checking without transactional layer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: redundant without?

@pepyakin
pepyakin dismissed their stale reviewJanuary 30, 2020 15:01

Significant change of the implementation after my look.

@cheme
cheme requested a review from NikVolf as a code ownerApril 14, 2020 15:42
@iorvethiorveth mentioned this pull request May 13, 2020
49 tasks
@chemecheme mentioned this pull request May 14, 2020
@cheme

Copy link
Copy Markdown
ContributorAuthor

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).

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_reviewPull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@cheme@gui1117@pepyakin@Demi-Marie@bkchr@gavofyork@svyatonik@shawntabrizi@kianenigma@gnunicorn@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

BREAKING Overlay transaction support. - #3263

Closed
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change
Closed

BREAKING Overlay transaction support.#3263
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change

Conversation

@cheme

@chemecheme commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

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

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

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/state-machine/src/basic.rs Outdated
Comment threadcore/state-machine/src/ext.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@gui1117

Copy link
Copy Markdown
Contributor

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

cheme commented Jul 31, 2019

Copy link
Copy Markdown
ContributorAuthor

Maybe start_transactional_layer

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

chemeand others added 2 commits July 31, 2019 14:54
Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>
@pepyakin

pepyakin commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

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.

Comment threadcore/state-machine/src/changes_trie/build.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@tomusdrwtomusdrw mentioned this pull request Jan 28, 2020

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First brief look.

Comment threadprimitives/io/src/lib.rs Outdated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if StorageTransactions will be a better name?

// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
(_, 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
};
// 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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
.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()))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: can be removed

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: use cases

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: us?

None
}

/// Push a value without checking without transactional layer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: redundant without?

@pepyakin
pepyakin dismissed their stale reviewJanuary 30, 2020 15:01

Significant change of the implementation after my look.

@cheme
cheme requested a review from NikVolf as a code ownerApril 14, 2020 15:42
@iorvethiorveth mentioned this pull request May 13, 2020
49 tasks
@chemecheme mentioned this pull request May 14, 2020
@cheme

Copy link
Copy Markdown
ContributorAuthor

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).

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_reviewPull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@cheme@gui1117@pepyakin@Demi-Marie@bkchr@gavofyork@svyatonik@shawntabrizi@kianenigma@gnunicorn@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

BREAKING Overlay transaction support. - #3263

Closed
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change
Closed

BREAKING Overlay transaction support.#3263
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change

Conversation

@cheme

@chemecheme commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

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

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

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/state-machine/src/basic.rs Outdated
Comment threadcore/state-machine/src/ext.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@gui1117

Copy link
Copy Markdown
Contributor

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

cheme commented Jul 31, 2019

Copy link
Copy Markdown
ContributorAuthor

Maybe start_transactional_layer

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

chemeand others added 2 commits July 31, 2019 14:54
Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>
@pepyakin

pepyakin commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

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.

Comment threadcore/state-machine/src/changes_trie/build.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@tomusdrwtomusdrw mentioned this pull request Jan 28, 2020

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First brief look.

Comment threadprimitives/io/src/lib.rs Outdated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if StorageTransactions will be a better name?

// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
(_, 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
};
// 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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
.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()))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: can be removed

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: use cases

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: us?

None
}

/// Push a value without checking without transactional layer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: redundant without?

@pepyakin
pepyakin dismissed their stale reviewJanuary 30, 2020 15:01

Significant change of the implementation after my look.

@cheme
cheme requested a review from NikVolf as a code ownerApril 14, 2020 15:42
@iorvethiorveth mentioned this pull request May 13, 2020
49 tasks
@chemecheme mentioned this pull request May 14, 2020
@cheme

Copy link
Copy Markdown
ContributorAuthor

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).

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_reviewPull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@cheme@gui1117@pepyakin@Demi-Marie@bkchr@gavofyork@svyatonik@shawntabrizi@kianenigma@gnunicorn@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

BREAKING Overlay transaction support. - #3263

Closed
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change
Closed

BREAKING Overlay transaction support.#3263
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change

Conversation

@cheme

@chemecheme commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

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

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

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/state-machine/src/basic.rs Outdated
Comment threadcore/state-machine/src/ext.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@gui1117

Copy link
Copy Markdown
Contributor

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

cheme commented Jul 31, 2019

Copy link
Copy Markdown
ContributorAuthor

Maybe start_transactional_layer

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

chemeand others added 2 commits July 31, 2019 14:54
Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>
@pepyakin

pepyakin commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

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.

Comment threadcore/state-machine/src/changes_trie/build.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@tomusdrwtomusdrw mentioned this pull request Jan 28, 2020

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First brief look.

Comment threadprimitives/io/src/lib.rs Outdated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if StorageTransactions will be a better name?

// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
(_, 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
};
// 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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
.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()))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: can be removed

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: use cases

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: us?

None
}

/// Push a value without checking without transactional layer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: redundant without?

@pepyakin
pepyakin dismissed their stale reviewJanuary 30, 2020 15:01

Significant change of the implementation after my look.

@cheme
cheme requested a review from NikVolf as a code ownerApril 14, 2020 15:42
@iorvethiorveth mentioned this pull request May 13, 2020
49 tasks
@chemecheme mentioned this pull request May 14, 2020
@cheme

Copy link
Copy Markdown
ContributorAuthor

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).

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_reviewPull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@cheme@gui1117@pepyakin@Demi-Marie@bkchr@gavofyork@svyatonik@shawntabrizi@kianenigma@gnunicorn@devops-parity
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

BREAKING Overlay transaction support. - #3263

Closed
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change
Closed

BREAKING Overlay transaction support.#3263
cheme wants to merge 203 commits into
paritytech:masterfrom
cheme:overlay_change

Conversation

@cheme

@chemecheme commented Jul 30, 2019

Copy link
Copy Markdown
Contributor

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

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

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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!

Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/sr-io/without_std.rs Outdated
Comment threadcore/state-machine/src/basic.rs Outdated
Comment threadcore/state-machine/src/ext.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadsrml/support/src/storage/mod.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/executor/src/wasm_executor.rs Outdated
Comment threadcore/state-machine/Cargo.toml Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@gui1117

Copy link
Copy Markdown
Contributor

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

cheme commented Jul 31, 2019

Copy link
Copy Markdown
ContributorAuthor

Maybe start_transactional_layer

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

chemeand others added 2 commits July 31, 2019 14:54
Co-Authored-By: Sergei Pepyakin <s.pepyakin@gmail.com>
@pepyakin

pepyakin commented Jul 31, 2019

Copy link
Copy Markdown
Contributor

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.

Comment threadcore/state-machine/src/changes_trie/build.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
Comment threadcore/state-machine/src/overlayed_changes.rs Outdated
@tomusdrwtomusdrw mentioned this pull request Jan 28, 2020

@pepyakinpepyakin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

First brief look.

Comment threadprimitives/io/src/lib.rs Outdated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if StorageTransactions will be a better name?

// 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
(_, 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
};
// 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).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment threadprimitives/state-machine/src/ext.rs Outdated
.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()))))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: can be removed

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: use cases

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: us?

None
}

/// Push a value without checking without transactional layer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: redundant without?

@pepyakin
pepyakin dismissed their stale reviewJanuary 30, 2020 15:01

Significant change of the implementation after my look.

@cheme
cheme requested a review from NikVolf as a code ownerApril 14, 2020 15:42
@iorvethiorveth mentioned this pull request May 13, 2020
49 tasks
@chemecheme mentioned this pull request May 14, 2020
@cheme

Copy link
Copy Markdown
ContributorAuthor

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).

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_reviewPull request needs code review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@cheme@gui1117@pepyakin@Demi-Marie@bkchr@gavofyork@svyatonik@shawntabrizi@kianenigma@gnunicorn@devops-parity