Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bump rust version #4985

Merged
merged 9 commits into from
Feb 21, 2024
Merged

bump rust version #4985

merged 9 commits into from
Feb 21, 2024

Conversation

mangas
Copy link
Contributor

@mangas mangas commented Nov 13, 2023

No description provided.

@mangas mangas force-pushed the filipe/bump-rust-version branch 2 times, most recently from 8d52ee4 to 28e41bc Compare November 20, 2023 15:06
@mangas mangas marked this pull request as ready for review November 30, 2023 13:40
@mangas mangas force-pushed the filipe/bump-rust-version branch 2 times, most recently from 34fee96 to 17b482a Compare November 30, 2023 14:42
@mangas mangas force-pushed the filipe/bump-rust-version branch 7 times, most recently from efadb3d to df077fb Compare December 7, 2023 15:16
@mangas mangas force-pushed the filipe/bump-rust-version branch 3 times, most recently from 7644cf1 to 2b491f7 Compare December 13, 2023 10:48
Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

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

Kudos! This is a huge amount of work. The main thing I'd like to see addressed are a couple comments and the clone in store_get_scoped. Other than that, I think it's ready to merge, but I am no wasm expert, and it would be great for @leoyvens to look at this, too.

graph/src/data_source/offchain.rs Outdated Show resolved Hide resolved
graph/src/runtime/asc_heap.rs Outdated Show resolved Hide resolved
graph/src/runtime/wasm/host_exports.rs Outdated Show resolved Hide resolved
runtime/test/src/test/abi.rs Outdated Show resolved Hide resolved
chain/near/src/trigger.rs Outdated Show resolved Hide resolved
core/src/subgraph/runner.rs Show resolved Hide resolved
)?
// This is not great in a hot path but otherwise the self ref would not
// be released for the next block. Would be good to find a better pattern here.
.map(|e| e.into_owned());
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change builds for me without requiring a clone:

Suggested change
.map(|e| e.into_owned());
let block_state = &mut self.as_mut().ctx.state;
let entity_option =
host_exports.store_get(block_state, entity_type.clone(), id.clone(), gas, scope)?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it compiles but breaks the tests because of the runtime borrow checking so I had to put it back

Copy link
Collaborator

Choose a reason for hiding this comment

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

I found a way around the clone, but it's ugly as hell. The clone basically undoes PR #4624. Do we really need interior mutability for WasmInstanceContext? Can't we get away with just plain &mut as we used to?

diff --git a/graph/src/runtime/wasm/module/context.rs b/graph/src/runtime/wasm/module/context.rs
index cc3733214..6502209c3 100644
--- a/graph/src/runtime/wasm/module/context.rs
+++ b/graph/src/runtime/wasm/module/context.rs
@@ -140,6 +140,11 @@ impl WasmInstanceContext {
         scope: GetScope,
     ) -> Result<AscPtr<AscEntity>, HostExportError> {
         let host_exports = self.as_ref().ctx.host_exports.cheap_clone();
+        let instrument = self.as_ref().ctx.instrument;
+        let host_metrics = self.as_ref().host_metrics.cheap_clone();
+        let debug_fork = self.as_ref().ctx.debug_fork.cheap_clone();
+        let logger = self.as_ref().ctx.logger.cheap_clone();
+
         let _timer = self
             .as_ref()
             .host_metrics
@@ -150,37 +155,29 @@ impl WasmInstanceContext {
         let entity_type: String =
             asc_get(&read_store, self.as_mut().asc_heap_mut(), entity_ptr, gas)?;
         let id: String = asc_get(&read_store, self.as_mut().asc_heap_mut(), id_ptr, gas)?;
+        let (mut heap_mut, mut state_mut) = RefMut::map_split(
+            self.as_mut(),
+            |WasmInstanceContextInner { ctx, asc_heap, .. }| {
+                (asc_heap.as_mut().unwrap(), &mut ctx.state)
+            },
+        );
         let entity_option = host_exports
-            .store_get(
-                &mut self.as_mut().ctx.state,
-                entity_type.clone(),
-                id.clone(),
-                gas,
-                scope,
-            )?
+            .store_get(&mut state_mut, entity_type.clone(), id.clone(), gas, scope)?
             // This is not great in a hot path but otherwise the self ref would not
             // be released for the next block. Would be good to find a better pattern here.
             .map(|e| e.into_owned());
 
-        if self.as_ref().ctx.instrument {
-            debug!(self.as_ref().ctx.logger, "store_get";
+        if instrument {
+            debug!(logger, "store_get";
                     "type" => &entity_type,
                     "id" => &id,
                     "found" => entity_option.is_some());
         }
-        let host_metrics = self.as_ref().host_metrics.cheap_clone();
-        let debug_fork = self.as_ref().ctx.debug_fork.cheap_clone();
 
         let ret = match entity_option {
             Some(entity) => {
                 let _section = host_metrics.stopwatch.start_section("store_get_asc_new");
-                // let entity = entity.to_owned().sorted_ref();
-                asc_new(
-                    store,
-                    self.as_mut().asc_heap_mut(),
-                    &entity.sorted_ref(),
-                    gas,
-                )?
+                asc_new(store, &mut *heap_mut, &entity.sorted_ref(), gas)?
             }
             None => match &debug_fork {
                 Some(fork) => {
@@ -194,12 +191,7 @@ impl WasmInstanceContext {
                         Some(entity) => {
                             let _section =
                                 host_metrics.stopwatch.start_section("store_get_asc_new");
-                            let entity = asc_new(
-                                store,
-                                self.as_mut().asc_heap_mut(),
-                                &entity.sorted(),
-                                gas,
-                            )?;
+                            let entity = asc_new(store, &mut *heap_mut, &entity.sorted(), gas)?;
                             self.store_set(store, gas, entity_ptr, id_ptr, entity)?;
                             entity
                         }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even removing the RefCell this borrow conflict persisted (but now at compile time). I fixed it by switching from Cow to Arc.

@mangas mangas mentioned this pull request Dec 15, 2023
3 tasks
@mangas mangas requested a review from lutter December 15, 2023 18:58
@leoyvens
Copy link
Collaborator

Is there any protection here against a mapping maxing out a CPU by running an infinite loop?

@mangas
Copy link
Contributor Author

mangas commented Dec 18, 2023

Is there any protection here against a mapping maxing out a CPU by running an infinite loop?

That will be added on a following PR, that's why the comments were disabled

@mangas mangas force-pushed the filipe/bump-rust-version branch 2 times, most recently from 451e1b1 to b579f0b Compare December 18, 2023 11:02
@leoyvens
Copy link
Collaborator

We might not want to rollout, and surely not release, without some type of protection against that in place. It's preferable that master is always in a state which we'd be comfortable releasing.

@mangas
Copy link
Contributor Author

mangas commented Dec 19, 2023

We might not want to rollout, and surely not release, without some type of protection against that in place. It's preferable that master is always in a state which we'd be comfortable releasing.

Agreed, yet this is a specific case for which I proposed we make an exception since the fix should be shortly along but should not be added in this PR, first because of the need to maintain the PR open in the meantime and second we are not going to release before the protection is in place so that's not an issue as discussed and agreed by the team

@mangas mangas force-pushed the filipe/bump-rust-version branch 2 times, most recently from 32e9cf7 to 6a79cb9 Compare December 19, 2023 10:56
@leoyvens
Copy link
Collaborator

You can put the fix in a separate PR that's based on this branch, the fix doesn't need to be blocked on this being merged.

@leoyvens
Copy link
Collaborator

Taking a look at the use of interior mutability in WasmInstanceContext, but after hacking a bit on this I do think we can avoid it.

The fundamental issue seems to be that when trying to call AscHeap, we end up needing a mutable borrow to both a WasmInstanceContext and a StoreContextMut. But one is inside the other with StoreContextMut<WasmInstanceContext>, so this results in a double mutable borrow.

One way around a double mutable borrow would be the interior mutability, but a more idiomatic solution might be:
impl AscHeap for StoreContextMut<'_, WasmInstanceContext> { ... }

I experimented with that here and it seemed possible. It also seems to avoid changing the definition of the AscHeap trait, and as a consequence hopefully means we can avoid merging the runtime crate with graph.

@leoyvens
Copy link
Collaborator

To be able to keep the crate separation, we might in graph the impl:
impl<H: AscHeap> AscHeap for StoreContextMut<'_, H> { ... }

And then in runtime:
impl AscHeap for WasmInstanceContext { ... }

@leoyvens
Copy link
Collaborator

The above might not quite work, unless we also either:

  1. Make one change to trait AscHeap, which is to make fn raw_new take &self instead of &mut self. This will require using some atomics in struct AscHeap but should work.
  2. 'Flatten' the fields of struct AscHeap into WasmInstanceContext, removing struct AscHeap as a separate struct.

So there are some details we need to hash out, but I'm happy to pair on this

@leoyvens
Copy link
Collaborator

leoyvens commented Feb 2, 2024

This is ready for another review, all comments addressed. I'll run it on an integration cluster.

@leoyvens
Copy link
Collaborator

This has been running in the integration cluster for a few days, no discrepancies.

@leoyvens leoyvens force-pushed the filipe/bump-rust-version branch 4 times, most recently from 370fcba to a15b1b5 Compare February 14, 2024 19:09
@leoyvens leoyvens merged commit 1b48099 into master Feb 21, 2024
7 checks passed
@leoyvens leoyvens deleted the filipe/bump-rust-version branch February 21, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants