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

[BUGFIX] Makes UpdatableTag updates lazy #1010

Merged
merged 2 commits into from
Jan 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 37 additions & 54 deletions packages/@glimmer/validator/lib/validators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,7 @@ export interface Tagged {
*
* @param tag
*/
export function value(tag: Tag): Revision {
if (DEBUG) {
// compute to cache the latest value, which will prevent us from doing
// invalid updates later on.
tag[COMPUTE]();
}

export function value(_tag: Tag): Revision {
return $REVISION;
}

Expand All @@ -71,27 +65,9 @@ export function value(tag: Tag): Revision {
* @param snapshot
*/
export function validate(tag: Tag, snapshot: Revision) {
if (DEBUG) {
IS_VALIDATING = true;
}

let isValid = snapshot >= tag[COMPUTE]();

if (DEBUG) {
IS_VALIDATING = false;

if (isValid) {
// compute to cache the latest value, which will prevent us from doing
// invalid updates later on.
tag[COMPUTE]();
}
}

return isValid;
return snapshot >= tag[COMPUTE]();
}

let IS_VALIDATING: boolean | undefined;

//////////

/**
Expand Down Expand Up @@ -140,9 +116,11 @@ class MonomorphicTagImpl implements MonomorphicTag {
private lastValue = INITIAL;

private isUpdating = false;
private subtag: Tag | null = null;
private subtags: Tag[] | null = null;

private subtag: Tag | null = null;
private subtagBufferCache: Revision | null = null;

[TYPE]: MonomorphicTagType;

constructor(type: MonomorphicTagTypes) {
Expand All @@ -160,26 +138,21 @@ class MonomorphicTagImpl implements MonomorphicTag {
this.lastChecked = ++$REVISION;
} else if (lastChecked !== $REVISION) {
this.isUpdating = true;

if (DEBUG) {
// In DEBUG, we don't cache while validating only, because it is valid
// update a tag between calling `validate()` and `value()`. Once you
// call `value()` on a tag, its revision is effectively locked in, and
// if you attempt to update it to a tag that is more recent it could
// break assumptions in our system. This is why the assertion exists in
// the static `update()` method below.
if (!IS_VALIDATING) {
this.lastChecked = $REVISION;
}
} else {
this.lastChecked = $REVISION;
}
this.lastChecked = $REVISION;

try {
let { subtags, subtag, revision } = this;
let { subtags, subtag, subtagBufferCache, lastValue, revision } = this;

if (subtag !== null) {
revision = Math.max(revision, subtag[COMPUTE]());
let subtagValue = subtag[COMPUTE]();

if (subtagValue === subtagBufferCache) {
revision = Math.max(revision, lastValue);
} else {
// Clear the temporary buffer cache
this.subtagBufferCache = null;
revision = Math.max(revision, subtagValue);
}
}

if (subtags !== null) {
Expand All @@ -198,27 +171,37 @@ class MonomorphicTagImpl implements MonomorphicTag {
return this.lastValue;
}

static update(_tag: UpdatableTag, subtag: Tag) {
static update(_tag: UpdatableTag, _subtag: Tag) {
if (DEBUG && _tag[TYPE] !== MonomorphicTagTypes.Updatable) {
throw new Error('Attempted to update a tag that was not updatable');
}

// TODO: TS 3.7 should allow us to do this via assertion
let tag = _tag as MonomorphicTagImpl;
let subtag = _subtag as MonomorphicTagImpl;

if (subtag === CONSTANT_TAG) {
tag.subtag = null;
} else {
if (
DEBUG &&
tag.lastChecked === $REVISION &&
(subtag as MonomorphicTagImpl)[COMPUTE]() > tag.lastValue
) {
throw new Error(
'BUG: attempted to update a tag with a tag that has a more recent revision as its value'
);
}

// There are two different possibilities when updating a subtag:
//
// 1. subtag[COMPUTE]() <= tag[COMPUTE]();
// 2. subtag[COMPUTE]() > tag[COMPUTE]();
//
// The first possibility is completely fine within our caching model, but
// the second possibility presents a problem. If the parent tag has
// already been read, then it's value is cached and will not update to
// reflect the subtag's greater value. Next time the cache is busted, the
// subtag's value _will_ be read, and it's value will be _greater_ than
// the saved snapshot of the parent, causing the resulting calculation to
// be rerun erroneously.
//
// In order to prevent this, when we first update to a new subtag we store
// its computed value, and then check against that computed value on
// subsequent updates. If its value hasn't changed, then we return the
// parent's previous value. Once the subtag changes for the first time,
// we clear the cache and everything is finally in sync with the parent.
tag.subtagBufferCache = subtag[COMPUTE]();
tag.subtag = subtag;
}
}
Expand Down
100 changes: 29 additions & 71 deletions packages/@glimmer/validator/test/validators-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,91 +74,49 @@ module('@glimmer/validator: validators', () => {
assert.ok(validate(tag, snapshot));
});

if (DEBUG) {
test('it throws an error when updating a tag that has been validated with a tag that is more recent', assert => {
let tag = createUpdatableTag();
let subtag = createUpdatableTag();

// First, we get a snapshot of the parent
let snapshot = value(tag);

// Then we dirty the currently unrelated subtag
dirty(subtag);

// Then we validate the parent tag. This should "lock in" the tag, because it
// means we are verifying that whatever calculation it represents has not changed
assert.ok(validate(tag, snapshot));

// Now, we attempt to update the parent with the subtag. The parent has been locked
// in, but the subtag has a more recent value, which means we have backflowed. This
// should throw an error.
assert.throws(
() => update(tag as any, subtag),
/Error: BUG: attempted to update a tag with a tag that has a more recent revision as its value/
);
});

test('it throws an error when updating a tag that has been invalidated AND whose value has been read with a tag that is more recent', assert => {
let tag = createUpdatableTag();
let subtag = createUpdatableTag();

// First, we get a snapshot of the parent
let snapshot = value(tag);

// Then we dirty the parent so it is out of date
dirty(tag);
test('it correctly buffers updates when subtag has a less recent value', assert => {
let tag = createUpdatableTag();
let subtag = createUpdatableTag();

// Then we dirty the currently unrelated subtag, so it has a more recent value
dirty(subtag);
// First, we dirty the parent tag so it is more recent than the subtag
dirty(tag);

// Then we attempt validate the parent tag. The validation will fail, and will
// NOT lock in the parent value just yet.
assert.notOk(validate(tag, snapshot));
// Then, we get a snapshot of the parent
let snapshot = value(tag);

// Then we get the parent tag's value, which does lock in the value.
snapshot = value(tag);
// Now, we update the parent tag with the subtag, and revalidate it
update(tag as any, subtag);

// Now, we attempt to update the parent with the subtag. The parent has been locked
// in, but the subtag has a more recent value, which means we have backflowed. This
// should throw an error.
assert.throws(
() => update(tag as any, subtag),
/Error: BUG: attempted to update a tag with a tag that has a more recent revision as its value/
);
});
assert.ok(validate(tag, snapshot), 'tag is still valid after being updated');

test('does not throw an error when updating a tag that has been invalidated AND whose value has NOT been read with a tag that is more recent', assert => {
let tag = createUpdatableTag();
let subtag = createUpdatableTag();
// Finally, dirty the subtag one final time to bust the buffer cache
dirty(subtag);

// First, we get a snapshot of the parent
let snapshot = value(tag);
assert.notOk(validate(tag, snapshot), 'tag is invalid after subtag is dirtied again');
});

// Then we dirty the parent so it is out of date
dirty(tag);
test('it correctly buffers updates when subtag has a more recent value', assert => {
let tag = createUpdatableTag();
let subtag = createUpdatableTag();

// Then we dirty the currently unrelated subtag, so it has a more recent value
dirty(subtag);
// First, we get a snapshot of the parent
let snapshot = value(tag);

// Then we attempt validate the parent tag. The validation will fail, and will
// NOT lock in the parent value just yet.
assert.notOk(validate(tag, snapshot));
// Then we dirty the currently unrelated subtag
dirty(subtag);

// Now we update the parent to the new subtag, BEFORE its value has been
// calculated. Since we know the value hasn't been calculated or locked into
// the cache, this update is valid.
update(tag as any, subtag);
// Now, we update the parent tag with the subtag, and revalidate it
update(tag as any, subtag);

// Then we get the parent tag's value, which does lock in the value.
snapshot = value(tag);
assert.ok(validate(tag, snapshot), 'tag is still valid after being updated');

// Do an unrelated bump (something else in the system changed)
bump();
// Finally, dirty the subtag one final time to bust the buffer cache
dirty(subtag);

// Caches were fully busted, but the value was still correct.
assert.ok(validate(tag, snapshot));
});
assert.notOk(validate(tag, snapshot), 'tag is invalid after subtag is dirtied again');
});

if (DEBUG) {
test('does not allow cycles on tags that have not been marked with ALLOW_CYCLES', assert => {
let tag = createUpdatableTag();
let subtag = createUpdatableTag();
Expand Down