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

Add RuntimeCell and RuntimeRefCell #11155

Closed
wants to merge 1 commit into from
Closed

Add RuntimeCell and RuntimeRefCell #11155

wants to merge 1 commit into from

Conversation

athei
Copy link
Member

@athei athei commented Apr 3, 2022

This PR adds a Sync wrapper around Cell and RefCell. This allows us to have interior mutability in non-mutable static variables. It is essentially a way to have global mutable state without resorting to spinlocks. This is safe because it is added to FRAME and we know that the runtime is single threaded.

As a first user I migrated the code added in #10808 to use RuntimeCell.

@athei athei added A0-please_review Pull request needs code review. B3-apinoteworthy C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Apr 3, 2022

type Layer = u32;
static NUM_LEVELS: AtomicU32 = AtomicU32::new(0);
static NUM_LEVELS: RuntimeCell<u32> = RuntimeCell::new(0);
Copy link
Member

Choose a reason for hiding this comment

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

As long as we have the native runtime, this isn't safe. The native runtime can be executed from different threads.

Copy link
Member Author

Choose a reason for hiding this comment

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

Really? Multiple threads operate on the same memory at the same time? Why would that be the case?

Copy link
Member

Choose a reason for hiding this comment

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

Native runtime. This static is shared within the entire context of the binary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure but you still don't call into any runtime code concurrently with more than one thread? That would make no sense.

Copy link
Member

Choose a reason for hiding this comment

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

Not the runtime itself, however you can call from different threads into the runtime. Imagine some thread importing a block and another validating an extrinsic. Both will call into the runtime

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume in the wasm case we spawn different instances for those calls which are then called in parallel.

To me it sounds like those calls need to be serialized to have the same behavior as with the wasm runtime.

Copy link
Member

Choose a reason for hiding this comment

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

How do you want to serialize them? The point is, we don't use any global context and if we use it it needs to be thread local.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I get it now. The native runtime recycles the memory from other calls while the wasm gets fresh memory. Yeah this makes this wrong, unfortunately.

@athei athei closed this Apr 3, 2022
@athei athei mentioned this pull request Apr 4, 2022
@athei athei deleted the at-runtime-globals branch May 18, 2022 10:43
@louismerlin louismerlin removed the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants