chore: update moddable to get snapshot fixes #3358

dckc · 2021-06-18T21:18:11Z

fixes #3350
fixes #2322
ref https://github.com/agoric-labs/moddable/tree/ag-testnet-4

We were previously at a97e1fb60dde. Moddable released the fixes re #3350 as well as metering for bigint, regex for #2322. Here we add test and bump the metering version. In the moddable repo, the top two commits (20ec3085, 29b71e09) are our metering tweaks, rebased. 2602c98c is the latest change from moddable that affects XS.

moddable/xs$ git log --graph --pretty=format:'%ad %h %s' --abbrev-commit --date=short a97e1.. .
* 2021-05-02 29b71e09 feat(xsnap): count additions and removals from maps, sets
* 2021-05-01 20ec3085 feat: maintain garbageCollectionCount for xsnap
* 2021-06-20 2602c98c quick change to preflight keys needed by mod
* 2021-06-19 71b46945 XS: too many mod keys will abort later
* 2021-06-19 2572a7bc XS snapshot missing bound functions callback
* 2021-06-18 fd6dcf52 XS: safer capture error stack
* 2021-06-17 028fcd45 XS snapshot: cleanup chunks after wrtiting
* 2021-06-15 969ba224 XS metering string from/to number conversions
* 2021-06-14 274c545f XS: metering bigint
* 2021-06-14 a14a5c03 XS: metering regexp
* 2021-06-14 b594c62a XS: metering changes
* 2021-06-09 b99a8268 add ArrayBuffer.fromBigInt

moddable/xs$ git diff -w --stat a97e1fb60dde.. .
 xs/includes/xs.d.ts       |  1 +
 xs/platforms/esp/xsHost.c |  8 ++++++++
 xs/sources/xsAPI.c        | 35 +++++++++++++++++++++++++++++++++--
 xs/sources/xsAll.h        |  3 +--
 xs/sources/xsBigInt.c     | 65 ++++++++++++++++++++++++++++++++++++++++++++++-------------------
 xs/sources/xsError.c      |  8 ++++++--
 xs/sources/xsFunction.c   | 35 -----------------------------------
 xs/sources/xsRun.c        | 20 +++++++++++++++-----
 xs/sources/xsSnapshot.c   | 21 ++++++++++++++++++++-
 xs/sources/xsre.c         | 19 +++++++++++++++++--
 10 files changed, 147 insertions(+), 68 deletions(-)

warner

Looks good, as far as I can tell. Agreed that we should wait (at least a little bit) for upstream to get the fixes, so we can remain as close the official sources as we can.

dckc · 2021-06-21T17:07:08Z

Moddable released their stuff as well as some metering stuff. PTAL.

warner · 2021-06-21T18:11:03Z

packages/xsnap/src/xsnap.c

@@ -84,7 +86,8 @@ static char* fxWriteNetStringError(int code);
 #define mxSnapshotCallbackCount 6
 txCallback gxSnapshotCallbacks[mxSnapshotCallbackCount] = {
 	fx_issueCommand, // 0
-	fx_Array_prototype_meter, // 1
+	// fx_Array_prototype_meter, // 1
+	NULL, // 1


Does this mean that we cannot load any snapshots that happened to include a reference to this function? Or, we can load them, but if they subsequently actually try to call it, we'll get a crash?

I'm wondering if it would be better to remove the callback slot entirely, rather than replacing it with a NULL, if we aren't really preserving snapshot compatibility across this change.

Right; the NULL isn't worth the risk.

fixed in f337f8a

warner · 2021-06-21T18:19:35Z

Huh, in xs/sources/xsRun.c, on the new line 313, I see a %%ld in an sprintf string.. is that right? Won't that print a literal %ld and then ignore the arguments?

agoric-labs/moddable@a97e1fb...29b71e0#diff-ae25655a5f4a58dd1f12052e9954cd9f499d9916057191ea0ac918835796792aR313-R317

warner · 2021-06-21T18:22:31Z

Other than xsRun.c and the xsnap.c question, the rest of the delta looks good.

dckc · 2021-06-21T18:25:06Z

Huh, in xs/sources/xsRun.c, on the new line 313, I see a %%ld in an sprintf string.. is that right?

That does look goofy. I don't think it affects us: It's in the #else part of an #ifdef mxMetering, and we have mxMetering turned on.

But I guess we owe a heads-up to moddable. (p.s. done.)

warner · 2021-06-21T19:50:42Z

That test failure suggests that our default CPU meter allowance might be too low.

kriskowal · 2021-06-21T21:57:44Z

packages/xsnap/src/xsnap.c

+	fx_print, // 1
+	fx_setImmediate, // 2
+	fx_gc, // 3
+	fx_performance_now, // 4
 	// fx_evalScript,


Are there any snapshots in databases that we care about continuing to be able to restore? We will eventually need to put tombstones in this table rather than renumbering syscalls.

NULL was my hand-wave at a tombstone. Both you and @warner called my bluff, in different directions (which is good!).

The moddable folks added fx_Function_prototype_bound to the middle rather than the end of the array in xsSnapshot.c; so unless we want to push back on that, we're already not in a good position to preserve snapshot compatibility across this change.

Are there any snapshots in databases that we care about continuing to be able to restore?

I think not, and @warner 's comment suggests likewise.

agreed, not yet

We'll need some sort of versioning marker sooner or later. I suspect we're going to need @agoric/xsnap to keep multiple binaries around, since all of (snapshot binary format, the callbacks table, metering behavior) will be highly dependent upon the binary. So once a vat starts executing with a given version of the xsnap binary, both its snapshots and it's future execution are forever bound to that one version (modulo whole-transcript replay to switch to a different version, which might work with consensus but we can figure out the details later).

kriskowal · 2021-06-21T22:00:08Z

packages/xsnap/test/test-xs-perf.js

+  // https://en.wikipedia.org/wiki/ReDoS
+  // http://www.owasp.org/index.php/OWASP_Validation_Regex_Repository
+  const result = await vat.evaluate(`
+  'aaaaaaaaa!'.match(/^(([a-z])+.)+/)


kriskowal · 2021-06-21T22:01:07Z

packages/xsnap/test/test-xs-perf.js

@@ -56,7 +63,23 @@ test('meter details', async t => {
    },
    'auxiliary (non-consensus) meters are available',
  );
-  t.is(meterType, 'xs-meter-7');
+  // @ts-ignore extra meters not declared on RunResult (yet?)


Might be worth tracking this.

I'm mentally tracking it as #3139 . I guess since I have to re-spin ci anyway, I'll note it in the comment.

fixed in 5fd6c6f

kriskowal · 2021-06-21T22:02:46Z

packages/xsnap/src/xsnap.c

@@ -859,11 +860,13 @@ void fxFreezeBuiltIns(txMachine* the)
 	mxPop();
 }

+#if 0


Is this permanently dead code? How do we decide whether to remove the pragma fence or remove the code?

Again, it's good that you call my bluff here.

This is the how to organize xsnap? issue, to my mind. I lean toward a separate xsnap-native submodule.

I guess I might as well delete this code rather than #if 0 it, but I wanted to prompt some discussion first.

fixed in e432531

moddable/xs$ git log --graph --pretty=format:'%ad %h %s' --abbrev-commit --date=short a97e1.. . * 2021-06-20 2602c98c quick change to preflight keys needed by mod * 2021-06-19 71b46945 XS: too many mod keys will abort later * 2021-06-19 2572a7bc XS snapshot missing bound functions callback * 2021-06-18 fd6dcf52 XS: safer capture error stack * 2021-06-17 028fcd45 XS snapshot: cleanup chunks after wrtiting * 2021-06-15 969ba224 XS metering string from/to number conversions * 2021-06-14 274c545f XS: metering bigint * 2021-06-14 a14a5c03 XS: metering regexp * 2021-06-14 b594c62a XS: metering changes * 2021-06-09 b99a8268 add ArrayBuffer.fromBigInt moddable/xs$ git diff -w --stat a97e1fb60dde.. . xs/includes/xs.d.ts | 1 + xs/platforms/esp/xsHost.c | 8 ++++++++ xs/sources/xsAPI.c | 35 +++++++++++++++++++++++++++++++++-- xs/sources/xsAll.h | 3 +-- xs/sources/xsBigInt.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++------------------- xs/sources/xsError.c | 8 ++++++-- xs/sources/xsFunction.c | 35 ----------------------------------- xs/sources/xsMapSet.c | 15 --------------- xs/sources/xsMemory.c | 2 +- xs/sources/xsRun.c | 20 +++++++++++++++----- xs/sources/xsSnapshot.c | 21 ++++++++++++++++++++- xs/sources/xsre.c | 19 +++++++++++++++++-- 12 files changed, 148 insertions(+), 84 deletions(-)

`fx_Array_prototype_meter` was obsolete but it still caused a link error.

We aren't really preserving snapshot compatibility across this change, since the bound callback got added to the middle of the list in xsSnapshot.c. So never mind holding a slot for `fx_Array_prototype_meter`.

... as shown in solo tests

This experimental metering design was removed from the XS sources Jun 14 in b594c62acaee .

dckc requested review from warner, kriskowal and michaelfig June 18, 2021 21:18

warner approved these changes Jun 18, 2021

View reviewed changes

dckc mentioned this pull request Jun 19, 2021

feat: demand-paged vats in solo, chain #2848

Merged

dckc marked this pull request as draft June 21, 2021 16:56

dckc force-pushed the moddable-xs-update branch from acd8bd4 to 5fc8605 Compare June 21, 2021 16:57

dckc requested a review from warner June 21, 2021 17:06

dckc marked this pull request as ready for review June 21, 2021 17:06

warner reviewed Jun 21, 2021

View reviewed changes

dckc force-pushed the moddable-xs-update branch from 5605c61 to eb483d5 Compare June 21, 2021 20:33

dckc enabled auto-merge June 21, 2021 20:49

kriskowal approved these changes Jun 21, 2021

View reviewed changes

dckc force-pushed the moddable-xs-update branch from e432531 to 4d04180 Compare June 21, 2021 22:37

dckc added 10 commits June 21, 2021 16:53

fix(xsnap): fxMeterHostFunction is no more

67e6a51

`fx_Array_prototype_meter` was obsolete but it still caused a link error.

test(xsnap): metering for regex, bigint

59e6bd4

chore: update moddable to re-integrate monitoring

7143cd1

test(xsnap): test that extra meters are non-zero

4af7080

fix(xsnap): don't risk NULL in gxSnapshotCallbacks

3a6ddbb

We aren't really preserving snapshot compatibility across this change, since the bound callback got added to the middle of the list in xsSnapshot.c. So never mind holding a slot for `fx_Array_prototype_meter`.

fix(xsnap): 1e7 was too small for crank meter limit

95c52ab

... as shown in solo tests

test(xsnap): update replay test for larger meteringLimit

3f739ce

docs(xsnap): cite instrumentation TODO by issue #

c69526d

chore(xsnap): prune all reference to fxPatchHostFunction

e0a6634

This experimental metering design was removed from the XS sources Jun 14 in b594c62acaee .

kriskowal force-pushed the moddable-xs-update branch from 4d04180 to e0a6634 Compare June 21, 2021 23:53

dckc merged commit 79a888e into master Jun 22, 2021

dckc deleted the moddable-xs-update branch June 22, 2021 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update moddable to get snapshot fixes #3358

chore: update moddable to get snapshot fixes #3358

dckc commented Jun 18, 2021 •

edited

Loading

warner left a comment

dckc commented Jun 21, 2021

warner Jun 21, 2021

dckc Jun 21, 2021

warner commented Jun 21, 2021

warner commented Jun 21, 2021

dckc commented Jun 21, 2021 •

edited

Loading

warner commented Jun 21, 2021

kriskowal Jun 21, 2021

dckc Jun 21, 2021

warner Jun 22, 2021

kriskowal Jun 21, 2021

kriskowal Jun 21, 2021

dckc Jun 21, 2021

dckc Jun 21, 2021

kriskowal Jun 21, 2021

dckc Jun 21, 2021

dckc Jun 21, 2021

chore: update moddable to get snapshot fixes #3358

chore: update moddable to get snapshot fixes #3358

Conversation

dckc commented Jun 18, 2021 • edited Loading

warner left a comment

Choose a reason for hiding this comment

dckc commented Jun 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warner commented Jun 21, 2021

warner commented Jun 21, 2021

dckc commented Jun 21, 2021 • edited Loading

warner commented Jun 21, 2021

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

dckc commented Jun 18, 2021 •

edited

Loading

dckc commented Jun 21, 2021 •

edited

Loading