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

chore: Update to upstream 3.8.7 #9

Merged
merged 175 commits into from
Apr 25, 2023
Merged

chore: Update to upstream 3.8.7 #9

merged 175 commits into from
Apr 25, 2023

Conversation

mhofman
Copy link

@mhofman mhofman commented Apr 24, 2023

Clean merge of tag 3.8.7

Refs: Agoric/agoric-sdk#7083

@mhofman mhofman requested a review from michaelfig April 24, 2023 20:14
@mhofman mhofman requested review from warner and removed request for michaelfig April 25, 2023 14:45
Copy link

@warner warner left a comment

Choose a reason for hiding this comment

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

ok, looks like the changes I was expecting

if (the->keyDelta > 0) {
txID keyDelta = (theCount > the->keyDelta) ? theCount : the->keyDelta;
txID keyCount = (the->keyCount + keyDelta) - the->keyOffset;
txSlot** keyArray = c_realloc(the->keyArray, keyCount * sizeof(txSlot*));
Copy link

Choose a reason for hiding this comment

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

Note for the future: this uses c_realloc() directly, unlike slots/chunks which go through an intermediate fxAllocateChunks/fxAllocateSlots, which xsnapPlatform.c overrides to track in the space meter. So the space used by keys won't be tracked.

This keyArray wants to use re-alloc, which is not quite as easy to wrap/track. But when we start trying to meter space allocation in earnest, we should consider creating an fxReallocKeys(), which can be controlled by the platform, and which can compare the old size (i.e. it can somehow learn the old size, maybe it should accept a third arg with the old size, maybe the->keyCount?) and new size, and apply the difference to the meter.

Copy link
Author

Choose a reason for hiding this comment

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

Good call! Where should we track this?

Copy link

Choose a reason for hiding this comment

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

@@ -1545,6 +1527,120 @@ txSlot* fxUnprojectSlot(txMachine* the, txSnapshot* snapshot, txSlot* slot)
return slot;
}

int fxUseSnapshot(txMachine* the, txSnapshot* snapshot)
Copy link

Choose a reason for hiding this comment

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

Right, this must take a snapshot in RAM, and initializes a (blank) txMachine struct with the contents, for the "create new machine from the old one but within the same process" operation.

Copy link
Author

@mhofman mhofman Apr 25, 2023

Choose a reason for hiding this comment

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

No the txSnapshot is basically the wrapper around the FILE struct that fxSnapshotRead uses in xsnap-worker.c so it basically wants to read back from disk. We could implement it otherwise by also storing the snapshot in memory, but that's require temporary allocation.

size = htonl(size);
mxThrowIf((*snapshot->write)(snapshot->stream, &size, 4));
mxThrowIf((*snapshot->write)(snapshot->stream, "CREA", 4));
mxThrowIf((*snapshot->write)(snapshot->stream, &creation, sizeof(txCreation)));
mxThrowIf((*snapshot->write)(snapshot->stream, &(the->profileID), sizeof(txID)));
mxThrowIf((*snapshot->write)(snapshot->stream, &(the->tag), sizeof(txInteger)));
Copy link

Choose a reason for hiding this comment

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

note to self: this is a snapshot compatibility break: the new snapshot needs to include space for both incrementalKeyCount and the->tag (which is really the number of keys currently allocated).

@mhofman mhofman merged commit 46bbad5 into Agoric Apr 25, 2023
@mhofman mhofman deleted the mhofman/update-xs-3-8-7 branch April 25, 2023 18:07
@mhofman
Copy link
Author

mhofman commented Apr 25, 2023

I realized I shouldn't have created this branch at the exact commit of 3.8.7 instead of a merge commit of 3.8.7 into the Agoric head to avoid the double merge commit graph (possible because I believe this repo doesn't enforce the branch to be up to date).

@warner
Copy link

warner commented Apr 25, 2023

oh, yeah, that graph is a bit gross. Oh well.

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.