Enable collection deletion without swapping in key objects #5641
Conversation
FUDCo
changed the title
| } | ||
| } | ||
| if (!hasWeakKeys) { | ||
| syscall.vatstoreSet(prefix('|entryCount'), '0'); |
There was a problem hiding this comment.
This adds a vatstoreSet to the "delete the whole collection" case that wasn't there before, right? That seems like a tiny regression.
What do you think about having the caller (clearInternal) retain responsibility for writing out this length=0, so it can continue to use the same if (!hasWeakKeys && !isDeleting) logic as before? In that case, clearInternal() would do something like:
{
let doMoreGC = false;
if (isDeleting || (matchAny(keyPatt) && matchAny(valuePatt))) {
doMoreGC = clearInternalFull();
} else {
for (const k of keys(keyPatt, valuePatt)) {
doMoreGC = deleteInternal(k) || doMoreGC;
}
}
if (!hasWeakKeys && !isDeleting) {
...| doMoreGC = | ||
| value.slots.map(vrm.removeReachableVref).some(b => b) || doMoreGC; | ||
| syscall.vatstoreDelete(dbKey); | ||
| if (dbKey[0] === 'r') { |
There was a problem hiding this comment.
This is coupling to the behavior of.. packages/store/src/patterns/encodePassable.js > makeEncodePassable()? I'm a bit nervous about bitrot if store decided to change their encoding scheme.
I obviously don't want to call decodeRemotable() on dbKey, that would defeat the purpose of this fix.
One option would be to create a separate instance of makeDecodePassable() whose only job is to report "reference or not", probably by counting whether the decodeRemotable passed in was called or not:
function dbKeyIsRemotable(dbKey) {
let isRemotable = false;
function decodeRemotable(rem) { isRemotable = true; return undefined; }
const decoder = makeDecodePassable({decodeRemotable});
decoder(dbKey);
return isRemotable;
}Another option is to add a unit test which explicitly checks the output of makeEncodePassable to see that the encoded key starts with 'r', and to add cross-pointing comments between the test and this line (so store ever changes its behavior, our test will fail, and when someone goes to update the test, they'll following the pointer here and update the === 'r' comparison at the same time).
A third option (the most efficient and maintainable) would be to change store to export an additional function like:
export const encodedIsRemotable = (encoded) => encoded[0] === 'r';
harden(encodedIsRemotable);plus a unit test in store. Not sure what @erights would think about that.
I'm undecided as to which I like better, but I know I want at least one of them.
There was a problem hiding this comment.
My first reaction was that this ship had already sailed, as we have various other code that has baked in understanding of the encoding, but closer inspection suggests that's not so (there is some code like that but it's already inside the store package where it belongs). Of the three options you propose, I think the first is gross and the second is icky, so I lean towards the third. There's already all kinds of other whacky stuff exported from store so I don't expect @erights to object too loudly. In the interest of moving things forward I'm inclined to follow the "ask for forgiveness rather than permission" strategy.
| @@ -233,12 +233,12 @@ const testUpgrade = async (t, defaultManagerType) => { | |||
|
|
|||
| for (let i = 1; i < NUM_SENSORS + 1; i += 1) { | |||
| const vref = durVref(i); | |||
| const impKref = impKrefs[i]; | |||
| // const impKref = impKrefs[i]; | |||
There was a problem hiding this comment.
Does the test also pass if you instead just delete the chunk of survivingDurables.push(10); ... above? I tried to arrange the test to let us delete adjustments as we fixed the bugs that necessitated them, and commenting out all of these checks makes the tests less strict than they need to be.
(of course #5636 disables this entire swath of checks, depending upon the order in which they land, but it would be nice if they were as accurate as possible for the day in the future that we reenable them)
There was a problem hiding this comment.
No. I tried fiddling with the test as you suggest here but either I'm not understand what you're proposing or what you're proposing doesn't work. I don't really understand what this test is actually doing, so...
Unless you have a more concrete thing to try, I'm going to leave this bit as is.
There was a problem hiding this comment.
bummer, ok, when you merge this with the #5636 commenting-out, maybe add a comment to the line 266 t.is(reachable, expI) line just saying disabled as part of #5641 so we can backtrack this to the changes that necessitated it
at some point I might try to walk through this test and see how much can be re-enabled
FUDCo
force-pushed
the
5053-more-efficient-collection-clear
branch
from
eaed688 to
2044afb
Compare
| @@ -233,12 +233,12 @@ const testUpgrade = async (t, defaultManagerType) => { | |||
|
|
|||
| for (let i = 1; i < NUM_SENSORS + 1; i += 1) { | |||
| const vref = durVref(i); | |||
| const impKref = impKrefs[i]; | |||
| // const impKref = impKrefs[i]; | |||
There was a problem hiding this comment.
bummer, ok, when you merge this with the #5636 commenting-out, maybe add a comment to the line 266 t.is(reachable, expI) line just saying disabled as part of #5641 so we can backtrack this to the changes that necessitated it
at some point I might try to walk through this test and see how much can be re-enabled
FUDCo
force-pushed
the
5053-more-efficient-collection-clear
branch
from
2044afb to
8ed6493
Compare
If collection clear is unconditional, allow clearing out the entire contents without deserializing the keys.
Fixes #5053