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

v6; more modular #374

Merged
merged 12 commits into from
Nov 6, 2024
Merged

v6; more modular #374

merged 12 commits into from
Nov 6, 2024

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Oct 12, 2024

This…

  • Removes the dependency on the Observable inspector
  • Removes the dependency on the Observable (notebook) standard library
  • Removes the pre-built ES module and UMD bundles in favor of a (simpler) source-only distribution
  • Pares down the README to focus on the Runtime API rather than embedding notebooks
  • Add a publish workflow
  • Renames the test workflow
  • Serialize asynchronous evaluation to improve performance framework#1469

: new Promise(resolve => resolve(variable._definition.call(value0))))
// Wait for the previous definition to compute before recomputing.
const promise = variable._promise = variable._promise
.then(init, init)
Copy link
Member Author

@mbostock mbostock Oct 12, 2024

Choose a reason for hiding this comment

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

This change means that when a variable is redefined, it will wait for the old version of the variable to settle (fulfill or reject) before running. However, there are some important nuances here…

First, variables reject before computing if they are stale (if (variable._version !== version)). This means that if a variable is redefined multiple times, we won’t waste time computing already-invalidated definitions. Second, new variables are created when code is edited rather than redefining existing variables; see Framework’s define, and note its call to main.variable to define a new variable whenever code is edited. This means that new (edited) code will not wait for old code to settle before running. However, unchanged code that is downstream of the edited code will still wait for the old code to settle, which might be confusing.

We could fix the last point in Framework by redefining any downstream code as new variables if it proves to be confusing. As a workaround, you can reload the page if you don’t wait for old code to run.

Copy link
Contributor

@Fil Fil Oct 14, 2024

Choose a reason for hiding this comment

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

A delay is OK (as in the case of awaiting a now-useless SQL query), but it is definitely confusing when the unchanged code is not settling at all.

A typical example is when I make a mistake writing a cell as a Promise that never resolves; fixing the mistake in the code block does not fix the state of the page.

Copy link
Member Author

Choose a reason for hiding this comment

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

So what do you suggest we do?

Copy link
Contributor

Choose a reason for hiding this comment

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

After playing a bit more with it, only the programmer error case mentioned above can be seen as a regression, ending up in a situation where you have to reload the page to resync the tree of variables with your code — whereas in main fixing the code would fix it.

As for suggestion, I wish I had an idea :) “Redefining any downstream code as new variables”, as you mentioned, maybe with an additional timeout(?).

But the additional complexity might not be worth it. And this is such a big win that if we have to choose I prefer this version.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could make it so that calls to variable.define cause the variable and any downstream variables not to wait, such that we’d only end up waiting in the case of generators yielding a new value. I can look to see how hard that will be.

@mbostock mbostock marked this pull request as ready for review October 12, 2024 21:19
@mbostock
Copy link
Member Author

Should we implement #374 (comment) before merging?

@Fil
Copy link
Contributor

Fil commented Oct 18, 2024

I think it would be better, yes. But it's a win even without it.

@mbostock
Copy link
Member Author

For #374 (comment) I think it’s possible by tracking the set of redefined variables prior to each runtime computation, and then re-initializing their variable._promise so that downstream variables don’t wait. Something like…

diff --git a/src/runtime.js b/src/runtime.js
index 63f0215..cdb8b5a 100644
--- a/src/runtime.js
+++ b/src/runtime.js
@@ -2,7 +2,7 @@ import {RuntimeError} from "./errors.js";
 import {generatorish} from "./generatorish.js";
 import {Module, variable_variable, variable_invalidation, variable_visibility} from "./module.js";
 import {noop} from "./noop.js";
-import {Variable, TYPE_IMPLICIT, no_observer, variable_stale} from "./variable.js";
+import {Variable, TYPE_IMPLICIT, no_observer, variable_stale, no_value} from "./variable.js";
 
 const frame = typeof requestAnimationFrame === "function" ? requestAnimationFrame
   : typeof setImmediate === "function" ? setImmediate
@@ -13,6 +13,7 @@ export function Runtime(builtins, global = window_global) {
   Object.defineProperties(this, {
     _dirty: {value: new Set},
     _updates: {value: new Set},
+    _redefines: {value: new Set},
     _precomputes: {value: [], writable: true},
     _computing: {value: null, writable: true},
     _init: {value: null, writable: true},
@@ -120,7 +121,17 @@ async function runtime_computeNow() {
     }
   });
 
+  // Compute the transitive closure of updating, reachable variables.
+  const redefines = new Set(this._redefines);
+  redefines.forEach(function(variable) {
+    if (variable._reachable) {
+      variable._outputs.forEach(redefines.add, redefines);
+      variable._promise = no_value;
+    }
+  });
+
   this._computing = null;
+  this._redefines.clear();
   this._updates.clear();
   this._dirty.clear();
 
diff --git a/src/variable.js b/src/variable.js
index 16bdb9b..7230896 100644
--- a/src/variable.js
+++ b/src/variable.js
@@ -192,6 +192,7 @@ function variable_defineImpl(name, inputs, definition) {
   // been exposed and we can avoid this extra work.)
   if (this._version > 0) ++this._version;
 
+  runtime._redefines.add(this);
   runtime._updates.add(this);
   runtime._compute();
   return this;

But I feel like it needs fairly extensive testing to make sure it doesn’t break something. And in some ways I’m wondering if “worse is better” — maybe it’s a “good” thing that the variables wait when being re-evaluated because it lets you know that the old code is still running? It’s a bit annoying in a notebook to reload, but it’s not as annoying to reload in Framework. And there’s no way for the runtime to interrupt invalidated code, so a reload is the only way to clear the leak.

@Fil
Copy link
Contributor

Fil commented Oct 21, 2024

I can live with the worse is better approach. If it becomes too annoying we'll revisit

@mbostock mbostock merged commit 138849d into main Nov 6, 2024
1 check passed
@mbostock mbostock deleted the mbostock/modular branch November 6, 2024 05:41
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.

2 participants