From faf94c7b4e44edcdf2a04506122cf8ba6a1587e3 Mon Sep 17 00:00:00 2001 From: Jonathan Kelley Date: Mon, 9 Jan 2023 13:50:33 -0800 Subject: [PATCH] fix: make miri pass on context api --- packages/core/src/bump_frame.rs | 13 +++-- packages/core/src/scheduler/wait.rs | 7 +-- packages/core/src/scope_arena.rs | 23 +++------ packages/core/src/scopes.rs | 14 ++---- packages/core/src/virtual_dom.rs | 2 +- packages/core/tests/suspense.rs | 74 ++++++++++++++++------------- 6 files changed, 65 insertions(+), 68 deletions(-) diff --git a/packages/core/src/bump_frame.rs b/packages/core/src/bump_frame.rs index 44ea4f2d6..147fc5f06 100644 --- a/packages/core/src/bump_frame.rs +++ b/packages/core/src/bump_frame.rs @@ -1,9 +1,9 @@ use crate::nodes::RenderReturn; use bumpalo::Bump; -use std::cell::Cell; +use std::cell::{Cell, UnsafeCell}; pub(crate) struct BumpFrame { - pub bump: Bump, + pub bump: UnsafeCell, pub node: Cell<*const RenderReturn<'static>>, } @@ -11,7 +11,7 @@ impl BumpFrame { pub(crate) fn new(capacity: usize) -> Self { let bump = Bump::with_capacity(capacity); Self { - bump, + bump: UnsafeCell::new(bump), node: Cell::new(std::ptr::null()), } } @@ -26,4 +26,11 @@ impl BumpFrame { unsafe { std::mem::transmute(&*node) } } + + pub(crate) fn bump(&self) -> &Bump { + unsafe { &*self.bump.get() } + } + pub(crate) unsafe fn bump_mut(&self) -> &mut Bump { + unsafe { &mut *self.bump.get() } + } } diff --git a/packages/core/src/scheduler/wait.rs b/packages/core/src/scheduler/wait.rs index 8ec1776c2..ac7cbcacc 100644 --- a/packages/core/src/scheduler/wait.rs +++ b/packages/core/src/scheduler/wait.rs @@ -31,7 +31,8 @@ impl VirtualDom { // If the task completes... if task.task.borrow_mut().as_mut().poll(&mut cx).is_ready() { // Remove it from the scope so we dont try to double drop it when the scope dropes - self.scopes[task.scope.0].spawned_tasks.remove(&id); + let scope = &self.scopes[task.scope.0]; + scope.spawned_tasks.borrow_mut().remove(&id); // Remove it from the scheduler tasks.try_remove(id.0); @@ -63,10 +64,10 @@ impl VirtualDom { if let Poll::Ready(new_nodes) = as_pinned_mut.poll_unpin(&mut cx) { let fiber = self.acquire_suspense_boundary(leaf.scope_id); - let scope = &mut self.scopes[scope_id.0]; + let scope = &self.scopes[scope_id.0]; let arena = scope.current_frame(); - let ret = arena.bump.alloc(match new_nodes { + let ret = arena.bump().alloc(match new_nodes { Some(new) => RenderReturn::Ready(new), None => RenderReturn::default(), }); diff --git a/packages/core/src/scope_arena.rs b/packages/core/src/scope_arena.rs index 86625f86c..41656fe9d 100644 --- a/packages/core/src/scope_arena.rs +++ b/packages/core/src/scope_arena.rs @@ -7,7 +7,6 @@ use crate::{ scopes::{ScopeId, ScopeState}, virtual_dom::VirtualDom, }; -use bumpalo::Bump; use futures_util::FutureExt; use std::{ mem, @@ -48,11 +47,10 @@ impl VirtualDom { })) } - fn acquire_current_scope_raw(&mut self) -> Option<*mut ScopeState> { - self.scope_stack - .last() - .copied() - .and_then(|id| self.scopes.get_mut(id.0).map(|f| f.as_mut() as *mut _)) + fn acquire_current_scope_raw(&self) -> Option<*const ScopeState> { + let id = self.scope_stack.last().copied()?; + let scope = self.scopes.get(id.0)?; + Some(scope.as_ref()) } pub(crate) fn run_scope(&mut self, scope_id: ScopeId) -> &RenderReturn { @@ -62,17 +60,10 @@ impl VirtualDom { self.ensure_drop_safety(scope_id); let mut new_nodes = unsafe { - let scope = self.scopes[scope_id.0].as_mut(); + self.scopes[scope_id.0].previous_frame().bump_mut().reset(); - // if this frame hasn't been intialized yet, we can guess the size of the next frame to be more efficient - if scope.previous_frame().bump.allocated_bytes() == 0 { - scope.previous_frame_mut().bump = - Bump::with_capacity(scope.current_frame().bump.allocated_bytes()); - } else { - scope.previous_frame_mut().bump.reset(); - } + let scope = &self.scopes[scope_id.0]; - // Make sure to reset the hook counter so we give out hooks in the right order scope.hook_idx.set(0); // safety: due to how we traverse the tree, we know that the scope is not currently aliased @@ -142,7 +133,7 @@ impl VirtualDom { let frame = scope.previous_frame(); // set the new head of the bump frame - let allocated = &*frame.bump.alloc(new_nodes); + let allocated = &*frame.bump().alloc(new_nodes); frame.node.set(allocated); // And move the render generation forward by one diff --git a/packages/core/src/scopes.rs b/packages/core/src/scopes.rs index 02d68a4ff..f77236f33 100644 --- a/packages/core/src/scopes.rs +++ b/packages/core/src/scopes.rs @@ -73,7 +73,7 @@ pub struct ScopeState { pub(crate) node_arena_1: BumpFrame, pub(crate) node_arena_2: BumpFrame, - pub(crate) parent: Option<*mut ScopeState>, + pub(crate) parent: Option<*const ScopeState>, pub(crate) id: ScopeId, pub(crate) height: u32, @@ -85,7 +85,7 @@ pub struct ScopeState { pub(crate) shared_contexts: RefCell>>, pub(crate) tasks: Rc, - pub(crate) spawned_tasks: FxHashSet, + pub(crate) spawned_tasks: RefCell>, pub(crate) borrowed_props: RefCell>>, pub(crate) attributes_to_drop: RefCell>>, @@ -111,14 +111,6 @@ impl<'src> ScopeState { } } - pub(crate) fn previous_frame_mut(&mut self) -> &mut BumpFrame { - match self.render_cnt.get() % 2 { - 1 => &mut self.node_arena_1, - 0 => &mut self.node_arena_2, - _ => unreachable!(), - } - } - /// Get the name of this component pub fn name(&self) -> &str { self.name @@ -139,7 +131,7 @@ impl<'src> ScopeState { /// If you need to allocate items that need to be dropped, use bumpalo's box. pub fn bump(&self) -> &Bump { // note that this is actually the previous frame since we use that as scratch space while the component is rendering - &self.previous_frame().bump + &self.previous_frame().bump() } /// Get a handle to the currently active head node arena for this Scope diff --git a/packages/core/src/virtual_dom.rs b/packages/core/src/virtual_dom.rs index 289cecf93..7369386b5 100644 --- a/packages/core/src/virtual_dom.rs +++ b/packages/core/src/virtual_dom.rs @@ -549,7 +549,7 @@ impl VirtualDom { loop { // first, unload any complete suspense trees for finished_fiber in self.finished_fibers.drain(..) { - let scope = &mut self.scopes[finished_fiber.0]; + let scope = &self.scopes[finished_fiber.0]; let context = scope.has_context::>().unwrap(); self.mutations diff --git a/packages/core/tests/suspense.rs b/packages/core/tests/suspense.rs index 775ca3d71..06bb7b1a4 100644 --- a/packages/core/tests/suspense.rs +++ b/packages/core/tests/suspense.rs @@ -5,42 +5,48 @@ use std::future::IntoFuture; use std::rc::Rc; use std::time::Duration; -#[tokio::test] -async fn it_works() { - let mut dom = VirtualDom::new(app); - - { - let mutations = dom.rebuild().santize(); - - // We should at least get the top-level template in before pausing for the children - // note: we dont test template edits anymore - // assert_eq!( - // mutations.templates, - // [ - // CreateElement { name: "div" }, - // CreateStaticText { value: "Waiting for child..." }, - // CreateStaticPlaceholder, - // AppendChildren { m: 2 }, - // SaveTemplate { name: "template", m: 1 } - // ] - // ); - - // And we should load it in and assign the placeholder properly - assert_eq!( - mutations.edits, - [ - LoadTemplate { name: "template", index: 0, id: ElementId(1) }, - // hmmmmmmmmm.... with suspense how do we guarantee that IDs increase linearly? - // can we even? - AssignId { path: &[1], id: ElementId(3) }, - AppendChildren { m: 1, id: ElementId(0) }, - ] - ); - } - +#[test] +fn it_works() { // wait just a moment, not enough time for the boundary to resolve - dom.wait_for_work().await; + tokio::runtime::Builder::new_current_thread() + .enable_time() + .build() + .unwrap() + .block_on(async { + let mut dom = VirtualDom::new(app); + + { + let mutations = dom.rebuild().santize(); + + // We should at least get the top-level template in before pausing for the children + // note: we dont test template edits anymore + // assert_eq!( + // mutations.templates, + // [ + // CreateElement { name: "div" }, + // CreateStaticText { value: "Waiting for child..." }, + // CreateStaticPlaceholder, + // AppendChildren { m: 2 }, + // SaveTemplate { name: "template", m: 1 } + // ] + // ); + + // And we should load it in and assign the placeholder properly + assert_eq!( + mutations.edits, + [ + LoadTemplate { name: "template", index: 0, id: ElementId(1) }, + // hmmmmmmmmm.... with suspense how do we guarantee that IDs increase linearly? + // can we even? + AssignId { path: &[1], id: ElementId(3) }, + AppendChildren { m: 1, id: ElementId(0) }, + ] + ); + } + + dom.wait_for_work().await; + }); } fn app(cx: Scope) -> Element {