Use Storage for Tracking Transactional Layers#10744

shawntabrizi · 2022-01-27T18:25:42Z

In progress...

Specification: #10806

frame/support/src/storage/mod.rs

shawntabrizi · 2022-01-28T04:41:46Z

Probably we should rename this from "transactional" which is just super confusing.

Maybe something more explicit like #[storage_overlays(n)]

athei · 2022-01-29T09:16:21Z

I still don't get why we want this. Shouldn't the transactional overhead incurred be part of the worst case benchmark. Meaning: Isn't the benchmark for an dispatchable already choosing the path that makes use of the most storage layers. So why do we need to track and cap it?

shawntabrizi · 2022-01-29T10:56:43Z

Number of storage layers is not something that is deterministic at the moment for a particular extrinsic. For example batch_all.

This limit can be used as a hint for the benchmarking to know how many layers to emulate for calculating weight.

athei · 2022-01-31T10:27:48Z

So this is to guard against a case where a storage heavy dispatchable is called with a lot of layers already created?

shawntabrizi · 2022-02-01T15:53:04Z

Yes, basically to know ahead of time the worst case number of layers that we could encounter, and potentially take care of this in weight calculation. Otherwise, we would have no idea, and we would need to assume worst case scenario, which would be like infinite.

Also, by default, we will make the number of layers 1.

athei · 2022-02-02T07:19:05Z

Yes, basically to know ahead of time the worst case number of layers that we could encounter, and potentially take care of this in weight calculation.

Maybe you can walk me through has this is supposed to work? Let's assume you are batch_all and limit the transaction depth to 3 on your transactional annotation. How would that help you with benchmarking batch_all? The weight does not depend on the number of layers but how many keys are written by the callee (called by batch_all) to said layers.

This reverts commit 5ae29f7.

ggwpez

I know it's WIP code but I think the comments still apply; was just curious.

ggwpez · 2022-02-02T16:57:09Z

frame/support/procedural/src/storage_layer.rs

+		#(#attrs)*
+		#vis #sig {
+			use #crate_::storage::{execute_with_storage_layer, has_storage_layer};
+			if has_storage_layer() {


So this prevents recursive storage layers?
Was that not the whole point of having a limit?

This would be an entirely different approach to storage layers, where rather than spawning new transactional layers per thing, we would just spawn at most one.

This would be the thing we would probably use by default across all pallet extrinsics.

ggwpez · 2022-02-02T17:00:57Z

frame/support/src/storage/mod.rs

-	thread_local! {
-		static TRANSACTION_LEVEL: RefCell<u32> = RefCell::new(0);
+	pub fn get_storage_layer() -> Layer {
+		crate::storage::unhashed::get_or_default::<Layer>(STORAGE_LAYER_KEY)


I'm not sure if there could be an optimization that we always return default for the top call in the call-stack, since the first call always has default we could get rid of the storage access there.

how do we know we are in a top call?

In any case, this is not a "real storage read" since it happens in memory always

ggwpez · 2022-02-02T17:07:27Z

frame/support/src/storage/mod.rs

+			return Err(())
+		}
+		// Cannot overflow because of check above.
+		set_storage_layer(&(existing_levels + 1));


If the get/set ends up wasting resources, we could add an Increment(key, limit) function to ParityDB.
Sql servers have this to reduce the number of queries.

this never gets written to DB, and thus is always in memory. I will let you see if you can rationalize why

ggwpez · 2022-02-02T17:11:33Z

frame/support/src/storage/mod.rs

+	fn dec_storage_layer() {
+		let existing_levels = get_storage_layer();
+		if existing_levels == 0 {
+			log::warn!(


Debug assert maybe?
When we extend the defensive_trait from @kianenigma we could have a proper solution here.

ggwpez · 2022-02-02T17:12:33Z

frame/support/src/storage/mod.rs

-	thread_local! {
-		static TRANSACTION_LEVEL: RefCell<u32> = RefCell::new(0);
+	pub fn get_storage_layer() -> Layer {
+		crate::storage::unhashed::get_or_default::<Layer>(STORAGE_LAYER_KEY)


You can debug assert that: if a value is read, it must be != 0.

ggwpez · 2022-02-02T17:13:46Z

frame/support/src/storage/mod.rs

+	}
+
+	pub fn has_storage_layer() -> bool {
+		get_storage_layer() > 0


This reduces to has_key(STORAGE_LAYER_KEY) as the storage layer value in the DB should never be 0.

has_key and get is the same complexity except has_key also needs to decode the storage into the right type. In this case, a u8 is trivial

ggwpez · 2022-02-02T17:29:01Z

frame/support/src/storage/mod.rs

-				require_transaction();
-				TransactionOutcome::Rollback(())
-			});
+			assert_ok!(execute_with_storage_layer(u8::MAX, || -> DispatchResult {


You can use assert_storage_noop! here and probably in others (eg require_transaction_should_return_error) as well.

i am not asserting a storage noop here though. I could modify storage in this call and it would still be valid. the test here is that it should return ok, and that within the function, the storage layer should exist.

stale · 2022-03-07T15:46:09Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

kianenigma · 2022-03-24T19:14:12Z

frame/support/procedural/src/storage_layer.rs

+use syn::{parse::Parse, ItemFn, LitInt, Result};
+
+struct StorageLayerLimit {
+	limit: u8,


why is this not a tuple struct?

kianenigma

Is there any written note on what should change about this PR?

shawntabrizi · 2022-03-24T19:42:42Z

I updated the comment to include the specification issue here: #10806

Sorry about that

kianenigma · 2022-03-28T19:31:35Z

frame/support/src/storage/mod.rs

+			log::warn!(
+				"We are underflowing with calculating transactional levels. Not great, but let's not panic...",
+			);


Suggested change

log::warn!(

"We are underflowing with calculating transactional levels. Not great, but let's not panic...",

);

frame_support::defensive!(

"We are underflowing with calculating transactional levels. Not great, but let's not panic...",

);

kianenigma · 2022-03-28T19:33:30Z

primitives/runtime/src/lib.rs

 			(ConsumerRemaining, ConsumerRemaining) |
-			(NoProviders, NoProviders) => true,
+			(NoProviders, NoProviders) |
+			(TooManyConsumers, TooManyConsumers) |


wish there would've been compiler warnings for not covering these..

shawntabrizi · 2022-04-02T20:29:49Z

breaking this down into parts, starting with:

#10808

use storage for tracking layers

b3b8d3b

github-actions bot added the A0-please_review Pull request needs code review. label Jan 27, 2022

propagate the limit error

be58fe0

shawntabrizi requested a review from athei as a code owner January 27, 2022 19:11

put check before creating layer

5ab450d

xlc reviewed Jan 27, 2022

View reviewed changes

frame/support/src/storage/mod.rs Outdated Show resolved Hide resolved

improve tests for storage reversion

baa34d5

shawntabrizi requested a review from bkchr January 28, 2022 04:42

update macro

b686b8c

shawntabrizi added 7 commits February 1, 2022 18:55

get rid of TransactionalOutcome

bd45a95

some renaming

b1c741f

more rename

6ce2dff

fix

12a9067

fmt

fb5448b

fix

f9c8fd4

better partial eq impl

5ae29f7

shawntabrizi added 3 commits February 2, 2022 12:37

Revert "better partial eq impl"

9f0ccd5

This reverts commit 5ae29f7.

fixes

9548a12

add back for now

f479b5c

kianenigma added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Feb 2, 2022

ggwpez reviewed Feb 2, 2022

View reviewed changes

shawntabrizi mentioned this pull request Feb 7, 2022

Add Limit to Tranasctional Layers #10808

Merged

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 7, 2022

kianenigma removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 8, 2022

kianenigma reviewed Mar 24, 2022

View reviewed changes

kianenigma reviewed Mar 28, 2022

View reviewed changes

shawntabrizi closed this Apr 2, 2022

Comments

Conversation

shawntabrizi commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shawntabrizi commented Jan 28, 2022

Uh oh!

athei commented Jan 29, 2022

Uh oh!

shawntabrizi commented Jan 29, 2022

Uh oh!

athei commented Jan 31, 2022

Uh oh!

shawntabrizi commented Feb 1, 2022

Uh oh!

athei commented Feb 2, 2022

Uh oh!

ggwpez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Mar 7, 2022

Uh oh!

kianenigma Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

shawntabrizi commented Mar 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shawntabrizi commented Apr 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shawntabrizi commented Jan 27, 2022 •

edited

Loading

kianenigma Mar 24, 2022 •

edited

Loading