Define state consistency within a revision by zetanumbers · Pull Request #223 · anp/moxie

zetanumbers · 2020-12-14T22:42:02Z

Update dependencies;
Update documentation;
Create formal definition in the docs;
Add new "strong" tests.

codecov · 2020-12-29T09:00:39Z

Codecov Report

Merging #223 (cfa7bec) into main (4d14a31) will decrease coverage by 0.54%.
The diff coverage is 42.36%.

@@            Coverage Diff             @@
##             main     #223      +/-   ##
==========================================
- Coverage   27.78%   27.24%   -0.55%     
==========================================
  Files          50       50              
  Lines       11886    11729     -157     
  Branches     6634     6564      -70     
==========================================
- Hits         3303     3195     -108     
- Misses       6391     6446      +55     
+ Partials     2192     2088     -104

Impacted Files	Coverage Δ
dom/examples/todo/src/filter.rs	`0.00% <ø> (ø)`
dom/examples/todo/src/item.rs	`23.29% <ø> (+1.04%)`	⬆️
src/runtime/runloop.rs	`68.42% <0.00%> (-4.92%)`	⬇️
src/runtime/context.rs	`37.70% <13.04%> (-18.75%)`	⬇️
src/runtime/var.rs	`24.13% <17.64%> (-23.49%)`	⬇️
src/runtime.rs	`23.67% <45.45%> (-0.77%)`	⬇️
src/lib.rs	`38.98% <80.00%> (-1.26%)`	⬇️
mox/src/lib.rs	`0.00% <0.00%> (-40.00%)`	⬇️
benches/core.rs	`75.00% <0.00%> (-15.00%)`	⬇️
dom/examples/ssr/src/main.rs	`28.81% <0.00%> (-8.48%)`	⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d14a31...cfa7bec. Read the comment docs.

anp

Sorry it took so long to get to this! I like this approach a lot, although I have some questions about the public API it exposes -- see my review comments.

anp · 2021-01-04T22:53:39Z

    {
+        // RunLoop always forces it's first revision?
+        // or maybe just check if current revision is 0
+        self.force();


If it's possible to remove the force() API, I'd prefer that. It's an additional branch at the top of each revision but I don't think this code isn't hot enough to care about that overhead.

anp · 2021-01-04T22:54:34Z

+    /// Runs the root closure once with access to the runtime context, returning
+    /// the result. `Waker` is set for the next `Revision`, which starts after
+    /// the start of the run.
+    pub fn run_once_with(&mut self, waker: Waker) -> Out {


Is it possible to make this the only API? Is there a reason we still need to cache the waker and support running without one?

anp · 2021-01-04T22:55:16Z

    /// call `Runtime::run_once` on a regular interval regardless.
-    pub fn set_state_change_waker(&mut self, wk: Waker) {
-        self.wk = wk;
+    pub fn set_state_change_waker(&mut self, waker: Waker) {


At one point we discussed removing this altogether in favor of deleting run_once and having run_once_with take a waker -- is that still an option here?

anp · 2021-01-04T22:58:28Z

+        rcs_write.revision.0 += 1;
+        rcs_write.pending_changes.store(false, std::sync::atomic::Ordering::Relaxed);
+        drop(rcs_write);


Could you extract this to a separate method like RevisionController::increment_revision? It'll make calls to execute() a little more verbose but we'll never risk blocking writes to state variables if something changes here, because the write guard can't escape the controller's method body.

anp · 2021-01-04T23:00:30Z

 }

+#[derive(Debug)]
+pub(crate) struct RevisionControlSystem {


nit: RevisionController

Can you move this type to another file, src/runtime/controller.rs?

anp · 2021-01-04T23:21:35Z

+        let rcs_read = self.rcs.read();
+        let current = rcs_read.revision;


Suggested change

let rcs_read = self.rcs.read();

let current = rcs_read.revision;

let current = rcs.revision();

anp · 2021-01-04T23:21:58Z

+
+    /// Returns a reference to the current commit.
+    pub fn current_commit(&mut self) -> &Commit<State> {
+        let current = self.rcs.read().revision;


Suggested change

let current = self.rcs.read().revision;

let current = self.rcs.revision();

anp · 2021-01-04T23:24:43Z

        <li>
            <a style="cursor: pointer;"
-             class={if *visibility == to_set { "selected" } else { "" }}
+             class={if *visibility.commit_at_root() == to_set { "selected" } else { "" }}


We shouldn't need to acquire a lock here to get the visibility for the revision, but I think this is a problem for you because of my past laziness.

At the top of this function is this attribute:

#[illicit::from_env(visibility: &Key<Visibility>)]

I think this needs to become

#[illicit::from_env(visibility: &Commit<Visibility>, set_visibility: &Key<Visibility>)]

Which will require exposing the current commit via https://github.com/anp/moxie/blob/main/dom/examples/todo/src/lib.rs#L53 as well.

anp · 2021-01-04T23:26:14Z

        crate::App::boot_fn(&[Todo::new("weeeee")], root.clone(), || {
-            let todo = &illicit::expect::<Key<Vec<Todo>>>()[0];
+            let todos_key = &illicit::expect::<Key<Vec<Todo>>>();
+            let todo = &todos_key.commit_at_root()[0];


Similarly to my other comment, I think we need to pass down a Commit<Vec<Todo>> from the App.

anp · 2021-01-04T23:27:21Z

@@ -35,8 +35,8 @@ pub fn toggle(default_checked: bool) -> Span {
 #[illicit::from_env(todos: &Key<Vec<Todo>>, visibility: &Key<Visibility>)]
 pub fn todo_list() -> Ul {


See my comments in other todomvc files, I think if you add Commit<Visibility> and Commit<Vec<Todo>> to the illicit environment then you can avoid all the commit_at_root calls in this file.

anp · 2021-01-04T23:31:02Z

A question: would this change be easier if RevisionControlState used an AtomicU64 to track the revision number rather than the Revision type itself?

Removed `commit_at_root` field of the `Key` bc of it being out of date.

increments, commits from the past revisions are lazely commited at first measurement

zetanumbers force-pushed the runtime_consistency branch from c0189ab to 1919a88 Compare December 25, 2020 08:58

zetanumbers mentioned this pull request Dec 26, 2020

Add web features to the parking_lot dependency #225

Closed

zetanumbers marked this pull request as ready for review December 26, 2020 15:05

zetanumbers changed the title ~~Reconsideration of the runtime logic~~ Define state consistency within a revision Dec 28, 2020

zetanumbers force-pushed the runtime_consistency branch from aef6d2c to 4dfb658 Compare December 29, 2020 08:20

zetanumbers mentioned this pull request Dec 29, 2020

Fix RunLoop behavior as an async Stream #216

Closed

zetanumbers force-pushed the runtime_consistency branch from b368d34 to 6febf59 Compare January 3, 2021 18:07

anp reviewed Jan 4, 2021

View reviewed changes

zetanumbers added 7 commits January 5, 2021 17:00

Initial fix of anp#197 anp#215 anp#220 anp#222 for moxie crate

772c697

Rollback public API

a51a0e6

Rollback Defer implementation for Key

b3fe8c3

Add poll_once for Runloop

167efaa

Docs update

9100c8d

Removed Deref implementation for Key;

ef032ec

Removed `commit_at_root` field of the `Key` bc of it being out of date.

Fix Var's commits so that after Revision

cfa7bec

increments, commits from the past revisions are lazely commited at first measurement

zetanumbers force-pushed the runtime_consistency branch from 6febf59 to cfa7bec Compare January 5, 2021 14:00

		let rcs_read = self.rcs.read();
		let current = rcs_read.revision;

	let rcs_read = self.rcs.read();
	let current = rcs_read.revision;
	let current = rcs.revision();

	let current = self.rcs.read().revision;
	let current = self.rcs.revision();

		@@ -35,8 +35,8 @@ pub fn toggle(default_checked: bool) -> Span {
		#[illicit::from_env(todos: &Key<Vec<Todo>>, visibility: &Key<Visibility>)]
		pub fn todo_list() -> Ul {

Conversation

zetanumbers commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

anp 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!

anp commented Jan 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zetanumbers commented Dec 14, 2020 •

edited

Loading

codecov Bot commented Dec 29, 2020 •

edited

Loading