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

fix(swingset): disable metering around GC-sensitive operations #3583

Closed
wants to merge 2 commits into from

Conversation

warner
Copy link
Member

@warner warner commented Aug 3, 2021

When a swingset kernel is part of a consensus machine, the visible state must
be a deterministic function of userspace activity. Every member kernel must
perform the same set of operations.

However we are not yet confident that the timing of garbage collection will
remain identical between kernels that experience different patterns of
snapshot+restart. In particular, up until recently, the amount of "headroom"
in the XS memory allocator was reset upon snapshot reload: the new XS engine
only allocates as much RAM as the snapshot needs, whereas before the snapshot
was taken, the RAM footprint could have been larger (e.g. if a large number
of objects we allocated and then released), leading to more "headroom".
Automatic GC is triggered by an attempt to allocate space which cannot be
satisfied by this headroom, so it will happen more frequently in the
post-reload engine than before the snapshot.

To accommodate differences in GC timing between kernels that are otherwise
operating in consensus, this commit introduces the "unmetered box": a span of
execution that does not count against the meter. We take all of the liveslots
operations that might be sensitive to the engine's GC behavior and put them
"in" the box.

This includes any code that calls deref() on the WeakRefs used in
slotToVal, because the WeakRef can change state from "live" to "dead" when
GC is provoked, which is sensitive to more than just userspace behavior.

closes #3458

@warner warner added the SwingSet package: SwingSet label Aug 3, 2021
@warner warner added this to the Testnet: Metering Phase milestone Aug 3, 2021
@warner warner self-assigned this Aug 3, 2021
@warner warner force-pushed the 3458-unmetered-box branch from 5c3e902 to bd30a24 Compare August 3, 2021 18:22
When a swingset kernel is part of a consensus machine, the visible state must
be a deterministic function of userspace activity. Every member kernel must
perform the same set of operations.

However we are not yet confident that the timing of garbage collection will
remain identical between kernels that experience different patterns of
snapshot+restart. In particular, up until recently, the amount of "headroom"
in the XS memory allocator was reset upon snapshot reload: the new XS engine
only allocates as much RAM as the snapshot needs, whereas before the snapshot
was taken, the RAM footprint could have been larger (e.g. if a large number
of objects we allocated and then released), leading to more "headroom".
Automatic GC is triggered by an attempt to allocate space which cannot be
satisfied by this headroom, so it will happen more frequently in the
post-reload engine than before the snapshot.

To accommodate differences in GC timing between kernels that are otherwise
operating in consensus, this commit introduces the "unmetered box": a span of
execution that does not count against the meter. We take all of the liveslots
operations that might be sensitive to the engine's GC behavior and put them
"in" the box.

This includes any code that calls `deref()` on the WeakRefs used in
`slotToVal`, because the WeakRef can change state from "live" to "dead" when
GC is provoked, which is sensitive to more than just userspace behavior.

closes #3458
@warner warner force-pushed the 3458-unmetered-box branch from bd30a24 to 9d5d652 Compare August 6, 2021 04:24
@warner warner marked this pull request as ready for review August 7, 2021 06:29
@warner warner requested a review from FUDCo August 7, 2021 06:29
@FUDCo
Copy link
Contributor

FUDCo commented Aug 7, 2021

This seems pretty terrible from a legibility perspective, and so I have to wonder if a notationally less awkward approach is possible (can't think of one, but we are collectively very clever...). Further, I'm more than a little concerned by the somewhat ad hoc nature of the selection of things to be wrapped. I'm wondering if there's some way to just meter user code and leave everything that's part of liveslots unmetered.

@warner
Copy link
Member Author

warner commented Aug 12, 2021

abandoned in favor of #3667

@warner warner closed this Aug 12, 2021
@warner warner removed the request for review from FUDCo August 12, 2021 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Liveslots should not meter GC-sensitive code paths
2 participants