fix: virtualize pool #4706

erights · 2022-03-01T06:05:25Z

Virtualize pool. Unlike #4707, this one does not virtualize vpool (the single pool) because it seems that the vpool need not be made persistent.

See #4707 (comment)

Note to reviewers: I may still be confused. I suspect that @Chris-Hibbert and I need to pair program this before it is good.

Chris-Hibbert · 2022-03-01T17:28:41Z

I'm expecting to see a test demonstrating that objects can be defined, saved, flushed from memory, and restored with behavior identical to the original. Is there a reason that's not appropriate here?

erights · 2022-03-21T22:33:15Z

At #4706 (comment) @Chris-Hibbert asks:

I'm expecting to see a test demonstrating that objects can be defined, saved, flushed from memory, and restored with behavior identical to the original. Is there a reason that's not appropriate here?

@FUDCo , should this PR test that? If so, how? Thanks.

erights · 2022-03-21T23:02:31Z

Converting back to draft because it is only a step towards #4707 . I'll let you know when #4707 is ready for review.

erights · 2022-03-23T04:00:44Z

Given @Chris-Hibbert 's comments at #4707 (comment) I put #4707 into draft status. Instead, it seems this PR #4706 is closer to what we want, so I'm declaring #4706 Ready to Review. Everyone, please review this one instead.

erights · 2022-03-23T04:12:08Z

Changed my mind again. Back to Draft until @Chris-Hibbert and I pair on this and it becomes ready. No need for others to review before then.

warner · 2022-03-23T05:42:22Z

At #4706 (comment) @Chris-Hibbert asks:

I'm expecting to see a test demonstrating that objects can be defined, saved, flushed from memory, and restored with behavior identical to the original. Is there a reason that's not appropriate here?

@FUDCo , should this PR test that? If so, how? Thanks.

We don't have a way to exercise that until #1848 / #4382 is done and we can upgrade vats. We've got test coverage on durable objects/collections in general via packages/SwingSet/ .. that will have to be enough until we get upgrade ready for use.

Although.. I suppose if you had a swingset-based test in which the virtual/durable collections were used to create some objects in vat A, then you carefully drop all references to them from RAM (leaving only exports and/or refs from virtualized data), then you wait a crank or two, then send a message back into vat A that references those objects.. that would exercise the code that rebuilds the objects from saved data.

The tricky part is knowing that the original objects really got GCed, which userspace doesn't get to see. You could add some temporary debug messages to liveslots.js while testing the test, to convince yourself that GC is really happening.

Running the workers under XS (config.defaultManagerType = 'xs-worker') is a good idea, that makes GC far more predictable.

Chris-Hibbert · 2022-03-23T16:13:59Z

We don't have a way to exercise that until #1848 / #4382 is done and we can upgrade vats. We've got test coverage on durable objects/collections in general via packages/SwingSet/ .. that will have to be enough until we get upgrade ready for use.

I was more interested in virtualization than durability. Can we page objects out and back in at this point?

Chris-Hibbert · 2022-03-23T16:43:03Z

packages/run-protocol/src/vpool-xyk-amm/pool.js

-            liquidityValueIn,
-          ),
-        );
+  const makePool = defineKind(


is the best documentation on defineKind the comment on defineKindInternal in virtualObjectManager.js? The parameter types don't seem to be declared in a way that's visible to VSCode and WebStorm, so I always have to find the docs before I can tell what arguments should be provided in what order.

48dbf0f#diff-4d09d78ca33d43e2584cfbf5adc00ed1caac51f1edbc10bae3d3c14320e56d30 will help

Chris-Hibbert

This looks fine and straightforward. I asked one question about how makeKind works.

Do we have the ability to demonstrate that virtualization can handle paging objects out and back in again?

I've asked before and it seems like the responses have assumed I was asking about durability and revival of the contract rather than the pool objects.

Chris-Hibbert · 2022-03-23T17:12:00Z

packages/run-protocol/src/vpool-xyk-amm/pool.js

-        );
+  const makePool = defineKind(
+    'pool',
+    (liquidityZcfMint, poolSeat, secondaryBrand) => {


I'm trying to understand defineKind. How does it know what parameters to provide to the init function? (i.e. If the pool doesn't declare them somewhere, what rules produce the parameters?)

defineKind doesn't "know" the parameters to init. The maker function calls init with whatever parameters it was given.

Chris-Hibbert · 2022-03-23T17:18:45Z

packages/run-protocol/src/vpool-xyk-amm/pool.js

+
+      /** @type {XYKPool} */
+      const pool = {
+        getLiquiditySupply: () => state.liqTokenSupply,


why is liqTokenSupply access through state, but liquidityIsuser, poolSeat, updater, etc. (line 126) are extracted and accessed as top-level values?

FUDCo · 2022-03-23T18:04:51Z

At #4706 (comment) @Chris-Hibbert asks:

I'm expecting to see a test demonstrating that objects can be defined, saved, flushed from memory, and restored with behavior identical to the original. Is there a reason that's not appropriate here?

@FUDCo , should this PR test that? If so, how? Thanks.

I don't think it should have to be the responsibility of users of the virtual object system to write tests to verify that the virtual object system works as advertised. The virtual objects implementation itself does have tests for that though.

Chris-Hibbert · 2022-03-23T18:33:50Z

I don't think it should have to be the responsibility of users of the virtual object system to write tests to verify that the virtual object system works as advertised. The virtual objects implementation itself does have tests for that though.

I'm willing to accept that it works as advertised. As a reviewer of a conversion, I'm trying to gain confidence that the right state has been captured. I'd feel a lot more confident if I saw that objects that had been paged out and in behaved the same as before. Without that I have to have confidence that I've identified all the state that is in use, and that I understand how the parts of makeKind interact in great detail.

FUDCo · 2022-03-23T19:20:11Z

I don't think it should have to be the responsibility of users of the virtual object system to write tests to verify that the virtual object system works as advertised. The virtual objects implementation itself does have tests for that though.

I'm willing to accept that it works as advertised. As a reviewer of a conversion, I'm trying to gain confidence that the right state has been captured. I'd feel a lot more confident if I saw that objects that had been paged out and in behaved the same as before. Without that I have to have confidence that I've identified all the state that is in use, and that I understand how the parts of makeKind interact in great detail

Ah, I misunderstood the intent of what you were trying to get at. That makes a lot of sense and seems like a thing that's genuinely important to verify.

erights · 2022-03-23T23:06:34Z

@FUDCo

Ah, I misunderstood the intent of what you were trying to get at. That makes a lot of sense and seems like a thing that's genuinely important to verify.

Is there a way to verify that? Should this PR do so? How?

turadg · 2022-04-22T17:26:25Z

Done in #5187

erights self-assigned this Mar 1, 2022

erights changed the base branch from master to markm-define-heap-kind March 1, 2022 07:03

erights mentioned this pull request Mar 1, 2022

fix: virtualize vPool as a facet #4707

Closed

erights requested review from FUDCo, turadg, warner and Chris-Hibbert March 1, 2022 07:08

erights marked this pull request as draft March 21, 2022 19:29

erights force-pushed the markm-multi-facet-pool branch from 3e0e502 to 72cfb21 Compare March 21, 2022 20:06

erights changed the base branch from markm-define-heap-kind to master March 21, 2022 20:07

erights marked this pull request as ready for review March 21, 2022 20:15

erights marked this pull request as draft March 21, 2022 20:16

erights force-pushed the markm-multi-facet-pool branch 2 times, most recently from fed23b2 to 7164f83 Compare March 21, 2022 20:40

erights changed the base branch from master to markm-virtualize-ERTP March 21, 2022 20:42

erights changed the base branch from markm-virtualize-ERTP to master March 21, 2022 20:45

erights changed the base branch from master to markm-virtualize-ERTP March 21, 2022 20:45

erights changed the base branch from markm-virtualize-ERTP to master March 21, 2022 20:47

erights changed the base branch from master to markm-virtualize-ERTP March 21, 2022 20:47

erights force-pushed the markm-multi-facet-pool branch from 1592e43 to 2cfe4a3 Compare March 21, 2022 22:16

erights marked this pull request as ready for review March 21, 2022 22:33

erights marked this pull request as draft March 21, 2022 23:01

erights force-pushed the markm-virtualize-ERTP branch 2 times, most recently from a4fb604 to a68c837 Compare March 22, 2022 19:53

Base automatically changed from markm-virtualize-ERTP to master March 22, 2022 20:47

fix: virtualize pool with vpool facet

8d7d290

erights force-pushed the markm-multi-facet-pool branch from 2cfe4a3 to 8d7d290 Compare March 23, 2022 03:35

erights marked this pull request as ready for review March 23, 2022 03:55

erights changed the title ~~fix: virtually virtualize pool~~ fix: virtualize pool Mar 23, 2022

erights marked this pull request as draft March 23, 2022 04:11

erights mentioned this pull request Mar 23, 2022

WIP: virtualize run-protocol pool object as example #4630

Closed

Chris-Hibbert reviewed Mar 23, 2022

View reviewed changes

turadg closed this Apr 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: virtualize pool #4706

fix: virtualize pool #4706

erights commented Mar 1, 2022 •

edited

Loading

Chris-Hibbert commented Mar 1, 2022

erights commented Mar 21, 2022

erights commented Mar 21, 2022

erights commented Mar 23, 2022

erights commented Mar 23, 2022

warner commented Mar 23, 2022 •

edited

Loading

Chris-Hibbert commented Mar 23, 2022

Chris-Hibbert Mar 23, 2022

turadg Mar 23, 2022

Chris-Hibbert left a comment

Chris-Hibbert Mar 23, 2022

FUDCo Mar 23, 2022

Chris-Hibbert Mar 23, 2022

FUDCo commented Mar 23, 2022

Chris-Hibbert commented Mar 23, 2022

FUDCo commented Mar 23, 2022

erights commented Mar 23, 2022

turadg commented Apr 22, 2022

fix: virtualize pool #4706

fix: virtualize pool #4706

Conversation

erights commented Mar 1, 2022 • edited Loading

Chris-Hibbert commented Mar 1, 2022

erights commented Mar 21, 2022

erights commented Mar 21, 2022

erights commented Mar 23, 2022

erights commented Mar 23, 2022

warner commented Mar 23, 2022 • edited Loading

Chris-Hibbert commented Mar 23, 2022

Chris-Hibbert Mar 23, 2022

Choose a reason for hiding this comment

turadg Mar 23, 2022

Choose a reason for hiding this comment

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Chris-Hibbert Mar 23, 2022

Choose a reason for hiding this comment

FUDCo Mar 23, 2022

Choose a reason for hiding this comment

Chris-Hibbert Mar 23, 2022

Choose a reason for hiding this comment

FUDCo commented Mar 23, 2022

Chris-Hibbert commented Mar 23, 2022

FUDCo commented Mar 23, 2022

erights commented Mar 23, 2022

turadg commented Apr 22, 2022

erights commented Mar 1, 2022 •

edited

Loading

warner commented Mar 23, 2022 •

edited

Loading