Skip to content

Commit

Permalink
Merge pull request #1010 from glimmerjs/bugfix/make-updatable-tag-upd…
Browse files Browse the repository at this point in the history 
…ates-lazy

[BUGFIX] Makes UpdatableTag updates lazy
  • Loading branch information
@rwjblue
rwjblue authored Jan 24, 2020
2 parents 761e78b + 1b06a4f commit da8888c
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 125 deletions.
91 changes: 37 additions & 54 deletions packages/@glimmer/validator/lib/validators.ts
View file
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
View file
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

0 comments on commit da8888c

Please sign in to comment.