From 4907d037fe7204c85f0ca24eb99f3637357f2677 Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Fri, 24 Jan 2020 13:50:17 -0800 Subject: [PATCH 1/2] [BUGFIX] Backports tag update buffering --- packages/@glimmer/reference/lib/validators.ts | 44 ++++++++++++++----- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/packages/@glimmer/reference/lib/validators.ts b/packages/@glimmer/reference/lib/validators.ts index 6e909db8db..d24a5b8bd5 100644 --- a/packages/@glimmer/reference/lib/validators.ts +++ b/packages/@glimmer/reference/lib/validators.ts @@ -129,9 +129,11 @@ export 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) { @@ -146,10 +148,18 @@ export class MonomorphicTagImpl implements MonomorphicTag { 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) { @@ -176,7 +186,7 @@ export class MonomorphicTagImpl implements MonomorphicTag { return this.lastValue; } - static update(_tag: UpdatableTag, subtag: Tag) { + static update(_tag: UpdatableTag, _subtag: Tag) { if (DEBUG) { assert( _tag[TYPE] === MonomorphicTagTypes.Updatable, @@ -186,17 +196,31 @@ export class MonomorphicTagImpl implements MonomorphicTag { // 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 { + // 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; - - // subtag could be another type of tag, e.g. CURRENT_TAG or VOLATILE_TAG. - // If so, lastChecked/lastValue will be undefined, result in these being - // NaN. This is fine, it will force the system to recompute. - tag.lastChecked = Math.min(tag.lastChecked, (subtag as any).lastChecked); - tag.lastValue = Math.max(tag.lastValue, (subtag as any).lastValue); } } From 8ba9f75eabf1ab0275196eec829afb3f2390aca5 Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Fri, 24 Jan 2020 13:55:01 -0800 Subject: [PATCH 2/2] add tests --- .../reference/test/validators-test.ts | 296 ++++++++++++++++++ 1 file changed, 296 insertions(+) create mode 100644 packages/@glimmer/reference/test/validators-test.ts diff --git a/packages/@glimmer/reference/test/validators-test.ts b/packages/@glimmer/reference/test/validators-test.ts new file mode 100644 index 0000000000..34fb230e16 --- /dev/null +++ b/packages/@glimmer/reference/test/validators-test.ts @@ -0,0 +1,296 @@ +import { DEBUG } from '@glimmer/local-debug-flags'; + +const module = QUnit.module; +const test = QUnit.test; + +import { + ALLOW_CYCLES, + CONSTANT_TAG, + CURRENT_TAG, + VOLATILE_TAG, + bump, + combine, + createTag, + createUpdatableTag, + dirty, + update, + validate, + value, +} from '..'; + +module('@glimmer/validator: validators', () => { + module('DirtyableTag', () => { + test('it can be dirtied', assert => { + let tag = createTag(); + let snapshot = value(tag); + + assert.ok(validate(tag, snapshot)); + + dirty(tag); + assert.notOk(validate(tag, snapshot)); + + snapshot = value(tag); + assert.ok(validate(tag, snapshot)); + }); + + if (DEBUG) { + test('it cannot be updated', assert => { + let tag = createTag(); + let subtag = createTag(); + + assert.throws( + () => update(tag as any, subtag), + /Error: Attempted to update a tag that was not updatable/ + ); + }); + } + }); + + module('UpdatableTag', () => { + test('it can be dirtied', assert => { + let tag = createUpdatableTag(); + let snapshot = value(tag); + + assert.ok(validate(tag, snapshot)); + + dirty(tag); + assert.notOk(validate(tag, snapshot)); + + snapshot = value(tag); + assert.ok(validate(tag, snapshot)); + }); + + test('it can be updated', assert => { + let tag = createUpdatableTag(); + let subtag = createUpdatableTag(); + + update(tag, subtag); + + let snapshot = value(tag); + assert.ok(validate(tag, snapshot)); + + dirty(subtag); + assert.notOk(validate(tag, snapshot)); + + snapshot = value(tag); + assert.ok(validate(tag, snapshot)); + }); + + test('it correctly buffers updates when subtag has a less recent value', assert => { + let tag = createUpdatableTag(); + let subtag = createUpdatableTag(); + + // First, we dirty the parent tag so it is more recent than the subtag + dirty(tag); + + // Then, we get a snapshot of the parent + let snapshot = value(tag); + + // Now, we update the parent tag with the subtag, and revalidate it + update(tag as any, subtag); + + assert.ok(validate(tag, snapshot), 'tag is still valid after being updated'); + + // Finally, dirty the subtag one final time to bust the buffer cache + dirty(subtag); + + assert.notOk(validate(tag, snapshot), 'tag is invalid after subtag is dirtied again'); + }); + + test('it correctly buffers updates when subtag has a more recent value', 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); + + // Now, we update the parent tag with the subtag, and revalidate it + update(tag as any, subtag); + + assert.ok(validate(tag, snapshot), 'tag is still valid after being updated'); + + // Finally, dirty the subtag one final time to bust the buffer cache + dirty(subtag); + + 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(); + + let snapshot = value(tag); + + update(tag, subtag); + update(subtag, tag); + + dirty(tag); + + assert.throws(() => validate(tag, snapshot)); + }); + + test('does allow cycles on tags that have been marked with ALLOW_CYCLES', assert => { + let tag = createUpdatableTag(); + let subtag = createUpdatableTag(); + + let snapshot = value(tag); + + ALLOW_CYCLES!.add(tag); + ALLOW_CYCLES!.add(subtag); + + update(tag, subtag); + update(subtag, tag); + + dirty(tag); + + assert.notOk(validate(tag, snapshot)); + }); + } + }); + + module('CombinatorTag', () => { + test('it can combine multiple tags', assert => { + let tag1 = createTag(); + let tag2 = createTag(); + + let combined = combine([tag1, tag2]); + + let snapshot = value(combined); + dirty(tag1); + assert.notOk(validate(combined, snapshot)); + + snapshot = value(combined); + dirty(tag2); + assert.notOk(validate(combined, snapshot)); + }); + + if (DEBUG) { + test('it cannot be dirtied', assert => { + let tag1 = createTag(); + let tag2 = createTag(); + + let combined = combine([tag1, tag2]); + + assert.throws( + () => dirty(combined as any), + /Error: Attempted to dirty a tag that was not dirtyable/ + ); + }); + + test('it cannot be updated', assert => { + let tag1 = createTag(); + let tag2 = createTag(); + + let combined = combine([tag1, tag2]); + + assert.throws( + () => update(combined as any, tag1), + /Error: Attempted to update a tag that was not updatable/ + ); + }); + } + }); + + module('ConstantTag', () => { + if (DEBUG) { + test('it cannot be dirtied', assert => { + assert.throws( + () => dirty(CONSTANT_TAG as any), + /Error: Attempted to dirty a tag that was not dirtyable/ + ); + }); + + test('it cannot be updated', assert => { + let subtag = createTag(); + + assert.throws( + () => update(CONSTANT_TAG as any, subtag), + /Error: Attempted to update a tag that was not updatable/ + ); + }); + } + }); + + module('VolatileTag', () => { + test('it is always invalid', assert => { + let snapshot = value(VOLATILE_TAG); + assert.notOk(validate(VOLATILE_TAG, snapshot)); + }); + + test('it ensures that any tags which it is combined with are also always invalid', assert => { + let tag2 = createTag(); + + let combined = combine([VOLATILE_TAG, tag2]); + + bump(); + + let snapshot = value(combined); + assert.notOk(validate(combined, snapshot)); + }); + + if (DEBUG) { + test('it cannot be dirtied', assert => { + assert.throws( + () => dirty(VOLATILE_TAG as any), + /Error: Attempted to dirty a tag that was not dirtyable/ + ); + }); + + test('it cannot be updated', assert => { + let subtag = createTag(); + + assert.throws( + () => update(VOLATILE_TAG as any, subtag), + /Error: Attempted to update a tag that was not updatable/ + ); + }); + } + }); + + module('CurrentTag', () => { + test('it is always the current revision', assert => { + let snapshot = value(CURRENT_TAG); + assert.ok(validate(CURRENT_TAG, snapshot)); + + let tag = createTag(); + dirty(tag); + + assert.notOk(validate(CURRENT_TAG, snapshot)); + }); + + test('it ensures that any tags which it is combined with are also always the current revision', assert => { + let tag2 = createTag(); + let combined = combine([CURRENT_TAG, tag2]); + + let snapshot = value(combined); + assert.ok(validate(combined, snapshot)); + + let otherTag = createTag(); + dirty(otherTag); + + assert.notOk(validate(combined, snapshot)); + }); + + if (DEBUG) { + test('it cannot be dirtied', assert => { + assert.throws( + () => dirty(CURRENT_TAG as any), + /Error: Attempted to dirty a tag that was not dirtyable/ + ); + }); + + test('it cannot be updated', assert => { + let subtag = createTag(); + + assert.throws( + () => update(CURRENT_TAG as any, subtag), + /Error: Attempted to update a tag that was not updatable/ + ); + }); + } + }); +});