Jank

[kornelski]

Major changes:

• Looping done using a regular for loop instead of recursion.
• Exceptions always interrupt batches, so exceptions aren't lost when multiple jobs throw exceptions (and behaviour with/without onError is more consistent)
• There's no explicit scheduling of multiple future frames. I'm not convinced that it's useful enough to warrant extra complexity it adds. I've hacked a sort-of-similar delay in defer for backwards compatibility only.
• And a big one: batches are timed and run for no longer than 16ms (where possible). This is intended to reduce jank. If the app schedules a lot of work, then the work gets spread over multiple frames to avoid jank and blocking of the main thread.

[wilsonpage]

This looks cool. We originally discussed this idea when we were first writing Fastdom. I remember being concerned that it could lead to events being run in a different order depending on the performance of the device, leading to device specific bugs. Although I'm willing to be proven wrong on this :)

I'm away on holiday until the 16th, so will take a proper look then.

On Thursday, March 5, 2015, Kornel notifications@github.com wrote:

The test failure is due to PhantomJS 1.x not supporting ES5.

— Reply to this email directly or view it on GitHub https://github.com/wilsonpage/fastdom/pull/52#issuecomment-77222887.

[kornelski]

Yes, it makes things work slightly differently depending on the speed of the device. The relative order is still preserved, although other events can happen in the meantime indeed. I'm not worried about this — that's something to be expected from async programming.

[wilsonpage]

Cool. If you guys at labs want to trial the web-app with this branch for a while and make sure all is running well, I'd be more comfortable merging :)

Do you think the 16ms timeout should be configurable? On 6 Mar 2015 01:14, "Kornel" notifications@github.com wrote:

Yes, it makes things work slightly differently depending on the speed of the device. The relative order is still preserved, although other events can happen in the meantime indeed. I'm not worried about this — that's something to be expected from async programming.

— Reply to this email directly or view it on GitHub https://github.com/wilsonpage/fastdom/pull/52#issuecomment-77488212.

[wilsonpage]

One other concern is that if you have a batch of (perhaps write) jobs that are required to construct a UI and some of those jobs are deferred to a later frame, the user may see a partially constructed UI being painted before the latter jobs are run? On 16 Mar 2015 05:33, wilsonpage@me.com wrote:

Cool. If you guys at labs want to trial the web-app with this branch for a while and make sure all is running well, I'd be more comfortable merging :)

Do you think the 16ms timeout should be configurable? On 6 Mar 2015 01:14, "Kornel" notifications@github.com wrote:

Yes, it makes things work slightly differently depending on the speed of the device. The relative order is still preserved, although other events can happen in the meantime indeed. I'm not worried about this — that's something to be expected from async programming.

— Reply to this email directly or view it on GitHub https://github.com/wilsonpage/fastdom/pull/52#issuecomment-77488212.

[wilsonpage]

Do you think we even need .defer()? It might be nice to kill it to simplify things. Do you you guys at labs have any real use-cases?

[wilsonpage]

Overall this patch looks really cool. I'm super impressed how much you've managed to simplify things! The jank code seems clever too, I like the way we just abandon things when we don't reach 30fps.

I'd like all the jsdoc updating and address a few formatting nits. If we can get something we're both happy with, this could warrant a v1.0 release :)

[kornelski]

Regarding .defer, I don't have a use-case for delaying by a number of frames.

Currently I've implemented it as "run this after finishing everything else", which is a nice-to-have. However, defer doesn't specify whether it's for reading or writing.

Perhaps defer could be generalized to being a priority setting on read/write?

[kornelski]

How about making onError, raf and timings fields in options object passed to the constructor? (and forbid overwriting of properties)

[wilsonpage]

@pornel the user never uses the constructor, just the fastdom singleton.

[wilsonpage]

RE onError: You're saying Safari doesn't surface errors thrown in a rAF?

[wilsonpage]

What about:

setTimeout(function() { throw error; });

[kornelski]

Viewing errors on a tethered device is not the problem – we're collecting errors in production from users' machines and send them to our error aggregator (something like Sentry).

[wilsonpage]

Perhaps we could dispatch a 'fastdomerror' error event on the document or something then?

[wilsonpage]

@pornel: "One other concern is that if you have a batch of (perhaps write) jobs that are required to construct a UI and some of those jobs are deferred to a later frame, the user may see a partially constructed UI being painted before the latter jobs are run?" Thoughts?

[kornelski]

To avoid partial renders I'd introduce a concept of a transaction, so that callbacks queued in the transaction are guaranteed to be executed without interruption.

Perhaps something like this:

fastdom.transaction(() => {
  fastdom.write(…);
  fastdom.write(…); // both will run in the same frame
});

but I'm not sure how big of a deal that is. Since writes are deferred you already have risk of e.g. unstyled DOM showing up.

In the FT app we have a tricky case where we'd like the on-screen rendering to be fully rendered at once, but we also use the same code to pre-render next page off-screen, and there we don't care about partial rendering, but we don't want that preloading to cause any scrolling jank.

[wilsonpage]

Hmmm interesting. I think we should keep the simplicity of .read() .write() but have the ability to opt-out of the timing logic if it's causing weirdness.

fastdom.config({ jankfree: false });

This kind of config could be set per-job as well globally

fastdom.write(() => { ... }, { jankfree: false }); 

[wilsonpage]

Or with my idea of fastdom.sandbox() you could create a psuedo fastdom interface with it's own config:

var sandbox = fastdom.sandbox();

sandbox.config({ jankfree: false });

sandbox.read(...);
sandbox.write(...);

sandbox.clear(); // clears all jobs scheduled by the sandbox

[wilsonpage]

This means you could have a kind of fastdom instance per component/controller, but it still use the fastdom global singlton under the hood.

[wilsonpage]

@pornel have you trialled this branch with the web-app yet? Did it fix your scrolling jank?

[wilsonpage]

@pornel one of the perf issue I've encountered a few times has been when devs need to measure something on app start, before window.onload. This causes a layout before the browser was intending on doing the initial layout.

It would be cool if fastdom could address this particular case, ie. deferring .reads() until after window.onload. No sure if that is a crazy idea or not...?

[kornelski]

The app is going to use it a couple of weeks. Improvements are noticeable on slower phones.

[kornelski]

I like the sandbox idea. We already need to simulate something like this in fruitmachine helper.

[wilsonpage]

In the animation example I'm recording a lower frame-rate with this branch than master. I guess animation is a different use case than simple view construction. If we do land something like this, I'd like it to be opt in.

I still really like the simplifications this branch makes to the library, so I'm going to land parts of it as a 'v1' release. I'd like more investigation as to whether the 'jank' logic improves things.

[kornelski]

So these changes have shipped in the ft app. Couple of cases we've found:

• defer is used to control CSS transitions, which must have a delay. I didn't think of that use-case (which in retrospect should have been obvious ^_^''), so this needs to be fixed. I misunderstood defer as low-priority write, but the priorities may better be handled explicitly.
• we had one place in the code which did write(() => write()) expecting write(() => defer()) behaviour.

I'm also going to change handling of onError to avoid rethrowing of exceptions, since Chrome DevTools handles rethrowing poorly (stack trace doesn't show the original place where the exception came from).

[wilsonpage]

I now feel this is logic is too opinionated for fastdom core. I think it should be possible to implement it as an extension.