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

[FEAT] Adds Effect Queue #1061

Closed
wants to merge 1 commit into from
Closed

[FEAT] Adds Effect Queue #1061

wants to merge 1 commit into from

Conversation

pzuraq
Copy link
Member

@pzuraq pzuraq commented Mar 22, 2020

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, 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.

Depends on #1060

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.
@pzuraq pzuraq force-pushed the feat/effect-queue branch from f10611c to 8c61278 Compare June 5, 2020 22:07
@rwjblue
Copy link
Member

rwjblue commented Jun 8, 2020

CI failure is linting related:

$ eslint . --cache --ext .js,.ts

/home/runner/work/glimmer-vm/glimmer-vm/packages/@glimmer/integration-tests/test/modifiers-test.ts
  1:9  error  Replace `·RenderTest,·jitSuite,·test,·Count,·tracked,·EmberishGlimmerComponent,·EmberishGlimmerArgs·` with `⏎··RenderTest,⏎··jitSuite,⏎··test,⏎··Count,⏎··tracked,⏎··EmberishGlimmerComponent,⏎··EmberishGlimmerArgs,⏎`  prettier/prettier

A quick yarn eslint --fix . --ext .js,.ts should resolve

* _prepended_ to any pre-existing effects. For instance, let's say that the
* queue started off in this state after our first render pass:
*
* A, B, C
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an ASCII graph for the tree of nodes

return true;
}

export class ScopeImpl<C extends JitOrAotBlock> implements PartialScope<C> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was moved to scope.ts since the environment.ts file was getting large and covering too many concerns.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will break this out into a separate PR

component!.showing = true;
this.rerender();

assert.deepEqual(inserts, ['outer', 'inner'], 'inner modifier insert called');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should reset inserts between each assertion

revalidate = () => this.effects.forEachNode(n => n.value.createOrUpdate());
}

export class EffectManager {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to EffectsManager

for (let phase of effectPhases) {
let queue = queues[phase];

scheduleEffects(phase, queue.revalidate);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a unit test to ensure that scheduleEffects can be called asynchronously and the rest of the EffectManager continues to work properly.


private didSetup = false;

createOrUpdate = memo(() => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this to run, and update ModifierManager to just include run

@pzuraq pzuraq mentioned this pull request May 19, 2021
@pzuraq
Copy link
Member Author

pzuraq commented May 20, 2021

Superceded by #1310

@pzuraq pzuraq closed this May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants