Rewrite Go JS core #166

PlasmaPower · 2023-10-08T23:59:55Z

This PR was co-authored with @rachel-bousfield so it should be reviewed by someone else.

This PR is currently a draft because it doesn't implement stylus-specific functionality like callbacks into Go (and comments out stylus tests for now). That said, I think it'd make sense to review this PR in its current state to make sure this approach makes sense before adding on Stylus callbacks. Everything implemented in this PR should be complete and ready for review.

Broadly speaking, this PR largely combines the go-stub wasm-libraries Go JS implementation and the JIT Go JS implementation into a single go-js library, and switches from an imperative programming style to a declarative style.

Before this PR, methods would be implemented with a match statement against the object ID and the method name:

    let value = match class {
        UINT8_ARRAY_ID => match args.get(0) {
            Some(JsValue::Number(size)) => DynamicObject::Uint8Array(vec![0; *size as usize]),
            _ => fail!("Go attempted to construct Uint8Array with bad args: {args:?}"),
        },
        DATE_ID => DynamicObject::Date,

This had two problems. First, code was grouped by things like "object construction" and "method calls", as opposed to being grouped by "filesystem" and "crypto" objects. Second, this relied heavily on assigning static IDs for everything, which didn't work out well as the system got more complicated. When manually assigning IDs, it's easy to mess up reference counting and such. The new approach separates the implementation of JS methods from the implementation details of the JS engine. Instead of the imperative style of a match statement for every possible method, this PR changes to a declarative style where objects and their methods are created as they would be in JS. This is slightly slower, but much clearer, and it's harder to make mistakes in this model. Here's an example:

/// in globals, we create the Date constructor
object.insert_func("Date", |_env, _go, _args| Ok(make_date_object()));

// make_date_object is defined as such:
fn make_date_object() -> JsValue {
    let date = JsObject::default();
    date.insert_func("getTimezoneOffset", |_env, _go, _args| {
        Ok(JsValue::Number(0.))
    });
    date.into()
}

The JS method implementations in runtime.rs never need to care about reference counting or object IDs. They can operate solely on high-level JS values.

tsahee

very initial. didn't go over everything yet.

tsahee · 2023-10-10T19:25:13Z

arbitrator/jit/src/gostack.rs

+        self.refresh(env, store)
+    }
+
+    /// Refreshes the stack pointer after potentially the Go runtime was potentially resumed.


potentially *2

tsahee · 2023-10-10T20:20:50Z

arbitrator/wasm-libraries/go-js/src/core.rs

+}
+
+impl PartialEq for JsValue {
+    fn eq(&self, other: &Self) -> bool {


Is this SameValueZero (worth a comment)?

also - maybe a small comment why you chose that

tsahee · 2023-10-10T20:26:03Z

arbitrator/wasm-libraries/go-js/src/core.rs

+            JsValue::String(x) => write!(f, "{x:?}"),
+            JsValue::Object(x) => write!(f, "{x:?}"),
+            JsValue::Uint8Array(x) => write!(f, "{x:?}"),
+            JsValue::Array(x) => write!(f, "{x:?}"),


type should also be displayed somehow..

tsahee · 2023-10-10T20:44:41Z

arbitrator/wasm-libraries/go-js/src/core.rs

+    }
+
+    /// Warning: this increments the reference count for the returned id
+    pub fn value_to_id(&self, value: JsValue) -> JsValueId {


function name should reflect the fact we add a refcount

Renamed to assign_id + a comment :)

tsahee · 2023-10-11T00:58:37Z

arbitrator/wasm-libraries/go-js/src/core.rs

+            // Treat all NaN values as equal
+            JsValue::Number(x) if x.is_nan() => JsValueEquality::Number(CANONICAL_NAN_BITS),
+            // Treat all zero values as equal
+            JsValue::Number(x) if *x == 0. => JsValueEquality::Number(0_f64.to_bits()),


I assume this works for both +0. and -0.? even so.. better to just check against both to make it apparent

rachel-bousfield · 2023-11-02T05:54:48Z

I've added some additional commits, and think the PR is pretty much good to go.

Highlights include

the new EVM Api, which introduces a much safer model for calling Go closures in Rust.
making call-and-recover an atomic operation, avoiding manual (and error-prone) management of the stack pointer.
fixing fs.write and undefined.Type()

codecov · 2023-11-02T05:59:12Z

Codecov Report

Merging #166 (6a135b5) into stylus (0509a98) will decrease coverage by 1.50%.
Report is 2 commits behind head on stylus.
The diff coverage is 81.29%.

Additional details and impacted files

@@            Coverage Diff             @@
##           stylus     #166      +/-   ##
==========================================
- Coverage   59.80%   58.31%   -1.50%     
==========================================
  Files          56      280     +224     
  Lines       11763    43102   +31339     
==========================================
+ Hits         7035    25135   +18100     
- Misses       4728    15502   +10774     
- Partials        0     2465    +2465

tsahee

This is still quite initial review and requires some talking.. specifically about when we want to panic and when we don't

We should also add (at least) two tests:

some version of the go-js test with necessary adaptations:
https://go.dev/src/syscall/js/js_test.go
a test for reference counting, which will create many objects, have them as fields in other objects/instances in arrays etc, probably pass them between different gofuncs, and have some hook to test we have many references in the middle and no unexpected references after all objects are destroyed,

tsahee · 2023-11-06T23:16:24Z

arbitrator/jit/src/gostack.rs

+        match String::from_utf8(bytes) {
+            Ok(s) => s,
+            Err(e) => {
+                let bytes = e.as_bytes();


was that intentional? won't it just print the error twice in the print below?

The second print calls Debug which includes extra info, and may change across updates to the stdlib. I've tweaked the message to provide the data in hex either way :)

arbitrator/jit/src/syscall.rs

tsahee · 2023-11-07T15:21:23Z

arbitrator/wasm-libraries/go-js/src/js_core.rs

+    }
+
+    /// Assigns an id for the given value, incrementing its reference count if already present.
+    pub fn assign_id(&self, value: JsValue) -> JsValueId {


Not mandatory in any way, but wanted to point out the original go-js implementation's version is "storeValue", which gets both the value and an address to store it. There isn't much representation through the code of value ids - only values that are stored/loaded as 8-bytes into/from memory. It has some downside, but makes the code overall less confusing, especially ref counting seems to make sense to me (because you only increase refcount when you actually store this reference)

tsahee · 2023-11-08T20:15:13Z

arbitrator/wasm-libraries/go-js/src/lib.rs

+    pub fn copy_bytes_to_go(&self, src: JsValueId, write_bytes: impl FnOnce(&[u8])) {
+        match self.values.id_to_value(src) {
+            JsValue::Uint8Array(array) => write_bytes(&array.lock()),
+            x => {


should return an error, caller should write 0 (instead of 1) to stack
should not panic

Agreed, refactored :)

tsahee · 2023-11-08T20:15:27Z

arbitrator/wasm-libraries/go-js/src/lib.rs

+        match self.values.id_to_value(dest) {
+            JsValue::Uint8Array(array) => write_bytes(&mut array.lock()),
+            x => {
+                panic!("Go attempted to call copyBytesToJs on invalid type: {x:?}");


should return an error, caller should write 0 (instead of 1) to stack
should not panic

Agreed, refactored :)

tsahee · 2023-11-08T20:27:43Z

arbitrator/wasm-libraries/go-js/src/lib.rs

+        let len = match self.values.id_to_value(array) {
+            JsValue::Array(array) => array.lock().len(),
+            JsValue::Uint8Array(array) => array.lock().len(),
+            x => {


exists for String
exists for functions
could exist for object
returns "undefined" for bool / number and doesn't panic

Go expects an integer rather than a type, so we should probs panic in our case if the result is not a number. I've updated the panic message to clarify this a bit, as well as added an implementation for String.

tsahee · 2023-11-08T21:35:40Z

arbitrator/jit/src/user/evm_api.rs

-                    env.js_state.pool.remove(id);
-                }
+                let js_env = &mut WasmerJsEnv::new(sp, &mut store, exports, &mut env.go_state)?;
+                let outs = js_state.call_stylus_func(api_id, method, args, js_env)?;
                respond.send(outs).unwrap();


in js_value_new (below):
is it o.k we're returning nil for error in case an error is returned? maybe better to create some simple error value?

Sorry which part / error? Did you mean to add this comment somewhere else?

tsahee · 2023-11-08T22:29:46Z

contracts

probably doesn't belong in this PR

This just bumps us to the latest stylus-contracts/stylus after a previous merge :)

tsahee · 2023-11-08T22:30:12Z

go-ethereum

probably doesn't belong in this PR

This just bumps us to the latest stylus/stylus after a previous merge :)

tsahee · 2023-11-08T22:31:10Z

system_tests/program_test.go

@@ -40,6 +39,7 @@ import (
 	"github.com/offchainlabs/nitro/validator/valnode"
 	"github.com/wasmerio/wasmer-go/wasmer"

+	"github.com/ethereum/go-ethereum/eth/tracers"


remove change.. or move tracers/js to be with the rest of go-ethereum imports

Fix tracing memory leak

cla-bot · 2023-11-15T17:09:02Z

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

cla-bot · 2023-11-17T04:44:49Z

We require contributors to sign our Contributor License Agreement. In order for us to review and merge your code, please sign the linked documents below to get yourself added. https://na3.docusign.net/Member/PowerFormSigning.aspx?PowerFormId=b15c81cc-b5ea-42a6-9107-3992526f2898&env=na3&acct=6e152afc-6284-44af-a4c1-d8ef291db402&v=2

Go Js Tests

Log address test

PlasmaPower added 3 commits October 8, 2023 17:39

Rewrite Go JS core

8ea4c0d

Merge branch 'stylus' into rewrite-go-js

bdcdee3

Fix missing restore_stack call after possible Go reentrancy

36197a6

PlasmaPower requested a review from tsahee October 8, 2023 23:59

cla-bot bot added the s label Oct 8, 2023

PlasmaPower assigned rachel-bousfield Oct 9, 2023

tsahee reviewed Oct 12, 2023

View reviewed changes

rachel-bousfield added 7 commits October 30, 2023 15:25

tweaks

7ec75c4

Merge remote-tracking branch 'stylus/stylus' into rewrite-go-js

cd04222

combine resume() and recover()

305df82

add stylus support for JIT

57a696f

add stylus support for Arbitrator

e0d91f1

add Dockerfile COPY

c2e94e4

fix fs.write

5acf6df

rachel-bousfield marked this pull request as ready for review November 2, 2023 05:55

address review comments

5a054be

tsahee reviewed Nov 8, 2023

View reviewed changes

Rachel Bousfield and others added 2 commits November 10, 2023 16:05

fix capture-hostio

df6022b

Merge pull request #175 from OffchainLabs/fix-capture-hostio

d27e030

Fix tracing memory leak

cla-bot bot removed the s label Nov 15, 2023

address review comments

72e6d21

rachel-bousfield added 5 commits November 17, 2023 22:26

official runtime tests

caedb15

arbitrator side + CI

4431219

fix test-js-runtime deps

9b10ae7

fix deps order

81d545e

add refcounting test

5046bd7

Merge pull request #177 from OffchainLabs/go-js-tests

ccd02d1

Go Js Tests

cla-bot bot added the s label Nov 28, 2023

rachel-bousfield added 3 commits November 28, 2023 14:06

log address test

755cca4

Merge pull request #181 from OffchainLabs/log-address-test

6a135b5

Log address test

Merge branch 'stylus' into rewrite-go-js

795cb51

rachel-bousfield merged commit cebfdd6 into stylus Nov 29, 2023
8 checks passed

rachel-bousfield deleted the rewrite-go-js branch November 29, 2023 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite Go JS core #166

Rewrite Go JS core #166

PlasmaPower commented Oct 8, 2023

tsahee left a comment

tsahee Oct 10, 2023

tsahee Oct 10, 2023

tsahee Oct 12, 2023

tsahee Oct 10, 2023

tsahee Oct 10, 2023

rachel-bousfield Nov 3, 2023

tsahee Oct 11, 2023

rachel-bousfield commented Nov 2, 2023

codecov bot commented Nov 2, 2023 •

edited

Loading

tsahee left a comment

tsahee Nov 6, 2023

rachel-bousfield Nov 17, 2023

tsahee Nov 7, 2023

tsahee Nov 8, 2023

rachel-bousfield Nov 17, 2023

tsahee Nov 8, 2023

rachel-bousfield Nov 17, 2023

tsahee Nov 8, 2023

rachel-bousfield Nov 17, 2023

tsahee Nov 8, 2023

rachel-bousfield Nov 17, 2023

tsahee Nov 8, 2023

rachel-bousfield Nov 17, 2023

tsahee Nov 8, 2023

rachel-bousfield Nov 17, 2023

tsahee Nov 8, 2023

cla-bot bot commented Nov 15, 2023

cla-bot bot commented Nov 17, 2023

Rewrite Go JS core #166

Rewrite Go JS core #166

Conversation

PlasmaPower commented Oct 8, 2023

tsahee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachel-bousfield commented Nov 2, 2023

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

tsahee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot bot commented Nov 15, 2023

cla-bot bot commented Nov 17, 2023

codecov bot commented Nov 2, 2023 •

edited

Loading