Skip to content

Commit

Permalink
[FEAT] Adds Effect Queue
Browse files Browse the repository at this point in the history
This PR prepares the VM for a refactor toward autotracking by extracting
modifiers and wrapping them in "effects". This is short for the term
[side-effect](https://en.wikipedia.org/wiki/Side_effect_(computer_science)),
which describes an action run within a function or computation that
doesn't technically have anything to do with the _output_ of that
computation. Modifiers are a form of side-effect, because they don't
directly affect the output of the VM - they schedule a change that
occurs _later_.

This also presented a problem for autotracking, which is why I started
with this PR. Autotracking operates on the premise that it is watching
state used within a computation - i.e. _synchronously_. Async actions,
like modifiers, violate the basic premise of autotracking. We were able
to model it with Tags using UpdatableTag, which does allow this type of
lazy structure, but updatable tags have been a major source of
performance issues and bugs in general.

Rather than having the VM be responsible for updating these async
actions, the way to handle this with autotracking would be to have
something _else_ handle it, specifically the place where the actual
computations _occur_. This is the Effect Queue.

\## How it works

The VM handles registration and destruction of effects, but the
environment is now responsible for updating them. Effects are scheduled
to run in the same position modifiers were previously, and all effects
in the queue are revalidated after each render.

Ordering is also still guaranteed, in that child effects will always be
triggered before parents. This is because new nodes are prepended to the
tree, and thus any new children are guaranteed to run before their older
parents. Due to the nature of the queue, it's possible that sibling
update order could change and be unpredictable.

\## Other options

We could use a tree that mirrors the VM tree instead of a queue, but
traversing that tree could end up being fairly expensive, especially if
there are few modifiers. I opted for the simpler solution for the time
being, and figured we could benchmark to see if there is a performance
impact currently, and if so what solutions are better.

Another option would be to make a n-ary tree that simply divides the
effects up evenly in memoization chunks. This might allow us to get some
wins in the general case, when most modifiers have not changed.
  • Loading branch information
Chris Garrett committed Mar 22, 2020
1 parent 4bf27f7 commit b217e8d
Show file tree
Hide file tree
Showing 12 changed files with 299 additions and 247 deletions.
21 changes: 10 additions & 11 deletions packages/@glimmer/integration-tests/lib/modifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@ import {
Dict,
ModifierManager,
GlimmerTreeChanges,
Destroyable,
DynamicScope,
VMArguments,
CapturedArguments,
} from '@glimmer/interfaces';
import { Tag } from '@glimmer/validator';
import { Tag, consumeTag } from '@glimmer/validator';

export interface TestModifierConstructor {
new (): TestModifierInstance;
Expand Down Expand Up @@ -47,6 +46,8 @@ export class TestModifierManager
}

install({ element, args, state }: TestModifier) {
consumeTag(args.tag);

if (state.instance && state.instance.didInsertElement) {
state.instance.element = element;
state.instance.didInsertElement(args.positional.value(), args.named.value());
Expand All @@ -56,22 +57,20 @@ export class TestModifierManager
}

update({ args, state }: TestModifier) {
consumeTag(args.tag);

if (state.instance && state.instance.didUpdate) {
state.instance.didUpdate(args.positional.value(), args.named.value());
}

return;
}

getDestructor(modifier: TestModifier): Destroyable {
return {
destroy: () => {
let { state } = modifier;
if (state.instance && state.instance.willDestroyElement) {
state.instance.willDestroyElement();
}
},
};
teardown(modifier: TestModifier): void {
let { state } = modifier;
if (state.instance && state.instance.willDestroyElement) {
state.instance.willDestroyElement();
}
}
}

Expand Down
9 changes: 6 additions & 3 deletions packages/@glimmer/interfaces/lib/runtime/environment.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { AttributeOperation } from '../dom/attributes';
import { AttrNamespace, SimpleElement, SimpleDocument } from '@simple-dom/interface';
import { ComponentInstanceState } from '../components';
import { ComponentManager } from '../components/component-manager';
import { Drop, Option } from '../core';
import { Drop, Option, SymbolDestroyable } from '../core';
import { GlimmerTreeChanges, GlimmerTreeConstruction } from '../dom/changes';
import { ModifierManager } from './modifier';

Expand All @@ -26,6 +26,10 @@ export interface Transaction {}
declare const TransactionSymbol: unique symbol;
export type TransactionSymbol = typeof TransactionSymbol;

export interface Effect extends SymbolDestroyable {
createOrUpdate(): void;
}

export interface Environment<Extra = unknown> {
[TransactionSymbol]: Option<Transaction>;

Expand All @@ -35,8 +39,7 @@ export interface Environment<Extra = unknown> {
willDestroy(drop: Drop): void;
didDestroy(drop: Drop): void;

scheduleInstallModifier(modifier: unknown, manager: ModifierManager): void;
scheduleUpdateModifier(modifier: unknown, manager: ModifierManager): void;
registerEffect(effect: Effect, transaction?: boolean): void;

begin(): void;
commit(): void;
Expand Down
2 changes: 1 addition & 1 deletion packages/@glimmer/interfaces/lib/runtime/modifier.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export interface ModifierManager<

// Convert the opaque token into an object that implements Destroyable.
// If it returns null, the modifier will not be destroyed.
getDestructor(modifier: ModifierInstanceState): Option<SymbolDestroyable | Destroyable>;
teardown(modifier: ModifierInstanceState): void;
}

export interface ModifierDefinition<
Expand Down
3 changes: 1 addition & 2 deletions packages/@glimmer/runtime/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,12 @@ export {
isWhitespace,
} from './lib/dom/helper';
export { normalizeProperty } from './lib/dom/props';
export { DefaultDynamicScope } from './lib/dynamic-scope';
export { PartialScopeImpl, DefaultDynamicScope } from './lib/scope';
export {
AotRuntime,
JitRuntime,
EnvironmentImpl,
EnvironmentDelegate,
ScopeImpl,
JitProgramCompilationContext,
JitSyntaxCompilationContext,
inTransaction,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
} from '@glimmer/interfaces';
import { VersionedPathReference, Reference } from '@glimmer/reference';
import { Tag, COMPUTE } from '@glimmer/validator';
import { ScopeImpl } from '../../environment';
import { PartialScopeImpl } from '../../scope';
import CurryComponentReference from '../../references/curry-component';
import {
CapturedNamedArgumentsImpl,
Expand Down Expand Up @@ -82,7 +82,9 @@ export const CheckCapturedArguments: Checker<CapturedArguments> = CheckInterface

export const CheckCurryComponent = wrap(() => CheckInstanceof(CurryComponentReference));

export const CheckScope: Checker<Scope<JitOrAotBlock>> = wrap(() => CheckInstanceof(ScopeImpl));
export const CheckScope: Checker<Scope<JitOrAotBlock>> = wrap(() =>
CheckInstanceof(PartialScopeImpl)
);

export const CheckComponentManager: Checker<ComponentManager<unknown>> = CheckInterface({
getCapabilities: CheckFunction,
Expand Down
83 changes: 45 additions & 38 deletions packages/@glimmer/runtime/lib/compiled/opcodes/dom.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
import { Reference, ReferenceCache, VersionedReference } from '@glimmer/reference';
import { Revision, Tag, isConst, isConstTag, valueForTag, validateTag } from '@glimmer/validator';
import { Revision, Tag, isConst, valueForTag, validateTag } from '@glimmer/validator';
import { check, CheckString, CheckElement, CheckOption, CheckNode } from '@glimmer/debug';
import { Op, Option, ModifierManager } from '@glimmer/interfaces';
import { $t0 } from '@glimmer/vm';
import {
ModifierDefinition,
InternalModifierManager,
ModifierInstanceState,
} from '../../modifier/interfaces';
import { ModifierDefinition } from '../../modifier/interfaces';
import { APPEND_OPCODES, UpdatingOpcode } from '../../opcodes';
import { UpdatingVM } from '../../vm';
import { Assert } from './vm';
Expand All @@ -16,6 +12,7 @@ import { CheckReference, CheckArguments, CheckOperations } from './-debug-strip'
import { CONSTANTS } from '../../symbols';
import { SimpleElement, SimpleNode } from '@simple-dom/interface';
import { expect, Maybe } from '@glimmer/util';
import { EffectImpl } from '../../environment';

APPEND_OPCODES.add(Op.Text, (vm, { op1: text }) => {
vm.elements().appendText(vm[CONSTANTS].getString(text));
Expand Down Expand Up @@ -86,12 +83,22 @@ APPEND_OPCODES.add(Op.CloseElement, vm => {

if (modifiers) {
modifiers.forEach(([manager, modifier]) => {
vm.env.scheduleInstallModifier(modifier, manager);
let d = manager.getDestructor(modifier);

if (d) {
vm.associateDestroyable(d);
}
let effect = new EffectImpl({
setup() {
manager.install(modifier);
},

update() {
manager.update(modifier);
},

teardown() {
manager.teardown(modifier);
},
});

vm.env.registerEffect(effect, true);
vm.associateDestroyable(effect);
});
}
});
Expand All @@ -117,35 +124,35 @@ APPEND_OPCODES.add(Op.Modifier, (vm, { op1: handle }) => {

operations.addModifier(manager, modifier);

let tag = manager.getTag(modifier);
// let tag = manager.getTag(modifier);

if (!isConstTag(tag)) {
vm.updateWith(new UpdateModifierOpcode(tag, manager, modifier));
}
// if (!isConstTag(tag)) {
// vm.updateWith(new UpdateModifierOpcode(tag, manager, modifier));
// }
});

export class UpdateModifierOpcode extends UpdatingOpcode {
public type = 'update-modifier';
private lastUpdated: Revision;

constructor(
public tag: Tag,
private manager: InternalModifierManager,
private modifier: ModifierInstanceState
) {
super();
this.lastUpdated = valueForTag(tag);
}

evaluate(vm: UpdatingVM) {
let { manager, modifier, tag, lastUpdated } = this;

if (!validateTag(tag, lastUpdated)) {
vm.env.scheduleUpdateModifier(modifier, manager);
this.lastUpdated = valueForTag(tag);
}
}
}
// export class UpdateModifierOpcode extends UpdatingOpcode {
// public type = 'update-modifier';
// private lastUpdated: Revision;

// constructor(
// public tag: Tag,
// private manager: InternalModifierManager,
// private modifier: ModifierInstanceState
// ) {
// super();
// this.lastUpdated = valueForTag(tag);
// }

// evaluate(vm: UpdatingVM) {
// let { manager, modifier, tag, lastUpdated } = this;

// if (!validateTag(tag, lastUpdated)) {
// vm.env.scheduleUpdateModifier(modifier, manager);
// this.lastUpdated = valueForTag(tag);
// }
// }
// }

APPEND_OPCODES.add(Op.StaticAttr, (vm, { op1: _name, op2: _value, op3: _namespace }) => {
let name = vm[CONSTANTS].getString(_name);
Expand Down
27 changes: 0 additions & 27 deletions packages/@glimmer/runtime/lib/dynamic-scope.ts

This file was deleted.

Loading

0 comments on commit b217e8d

Please sign in to comment.