fastdom.read seems unneeded and sometimes creates problems with third party scripts.
aFarkas opened this issue · 13 comments
I'm not sure whether I went nuts, but I think there is a substantial reasoning error with fastdom.read (not fastdom.write, which is awesome). In case I'm wrong, it would be great if you could explain this in depth.
fastdom is all about separating reads and writes. To do this efficiently all writes can be moved using a rAF. This way all read happens at once and all writes are scheduled at a time right before the browser calculates styles and layout naturally. This already separates efficiently all reads from all writes.
fastdom additionally also schedules all reads using rAF and separates the reads from the writes inside this "animation frame".
In my opinion this is not only unnecessary. It is dangerous as soon as any third party script is doing writes without using fastdom. By moving all reads at a later point right before normal style/layout calculation is done, the chance that styles are invalidated by another script are higher than executing the reads at any point before. In fact the chance that fastdom.read interferes with a third party script is nearly 100% as soon as the script is using requestAnimationFrame.
I would suggest to depreciate the use fastdom.read.
To proof this I modifed your demo. You can see it here:
http://afarkas.github.io/fastdom/examples/aspect-ratio.html
First simple observation:
There is no significant performance difference between "Run with FastDom" and "Run with FastDom - without read".
Second observation:
If you check the animate checkbox and then click "Run with FastDom" you will see the following layout thrashing:
Of course you can say, this animation script isn't using fastdom itself and therefore layout thrashing can always happen, but I think it is foreseeable and best practice to put dom/layout writes, especially for animations, in a rAF.
All this leads to a simple rule, never put your reads into a rAF and try to put all your writes into a rAF.
And here is a screenshot using fastdom.write without fastdom.read with checked animate checkbox:
+1 - I wonder why I just can't use rAF instead of fastdom WRITE?
This some interesting research thanks!
I still think fastdom.read() is required for some cases:
fastdom.write(() => {
element.textContent = 'long string ...';
fastdom.read(() => {
var height = element.clientHeight;
// do stuff with height ...
});
});I'm not sure how else we can safely read the new clientHeight value without it interleaving with other potential write operations.
FastDOM makes the assumption that you have control over all the scripts running in your app. It's probably not advisable to use if you have third-party scripts reading or writing to the DOM, as then global scheduling is polluted and all gains will be lost :(
I hope I have understood fully :)
Like most libraries, sometimes it's better to craft your own solution if you've identified a fast path specific to you application.
I'm not sure how else we can safely read the new clientHeight value without it interleaving with other potential write operations.
a) fastdom.read should be only used from inside a rAF or a fastdom.write. Otherwise it is never needed.
b) Then schedule fastdom.read tasks using setImmediate/setTimeout
c) For safety: If write tasks are run before reads (scheduled with setTimeout) flush them as an exception inside a rAF, right before any writes.
It's basically a saver/better way of:
fastdom.write(() => {
element.textContent = 'long string ...';
setTimeout(() => {
var height = element.clientHeight;
// do stuff with height ...
});
});FastDOM makes the assumption that you have control over all the scripts running in your app. It's probably not advisable to use if you have third-party scripts reading or writing to the DOM, as then global scheduling is polluted and all gains will be lost :(
I would disagree on multiple aspects.
a) It is not black or white. Of course just one read at a wrong position isn't worse than multiple sequenced reads at the same position. But reducing the number of alternating read/writes is always a win.
b) The assumption doesn't fit the reality of 99% of all websites.
c) it is possible to make fastdom play nice with third party scripts together.
About c):
As said currently fastdom is moving all reads and writes at the same spot and only separates inside this spot. This means FastDOM automatically always creates compatibility issues with third party scripts using requestAnimationFrame. But this shortcoming is not needed to accomplish the separation task efficiently.
As a bonus, if we can agree on the following best practice:
a) writes should happen inside a rAF
b) reads should not happen inside a rAF (Animating the scrollTop/scrollLeft position is the only read, that should be tolerated)
Developers can have multiple third party scripts using this technique without any clash and fastdom is just am optional tool to make developers live easier.
Here is a simple implementation of what I mean:
(function(){
'use strict';
var rAF = window.requestAnimationFrame || setTimeout;
var Fastdom = function(){
this._reads = [];
this._writes = [];
this._isWriting = false;
this._waitsForRead = false;
this._waitsForWrite = false;
this._flushWrites = this._flushWrites.bind(this);
this._flushReads = this._flushReads.bind(this);
};
Fastdom.prototype = {
read: function(fn, ctx, args){
if(this._isWriting){
this._reads.push(arguments);
if(!this._waitsForRead){
this._waitsForRead = true;
setTimeout(this._flushReads);
}
} else {
fn.apply(ctx || null, args);
}
},
write: function(fn, ctx, args){
this._writes.push(arguments);
if(!this._waitsForWrite){
this._waitsForWrite = true;
rAF(this._flushWrites);
}
},
writer: function(fn, ctx){
var that = this;
return function(){
that.write(fn, ctx || this, arguments);
};
},
reader: function(fn, ctx){
var that = this;
return function(){
that.read(fn, ctx || this, arguments);
};
},
_flush: function(list){
var fns;
while(list.length){
fns = list.shift();
fns[0].apply(fns[1] || null, fns[2] || []);
}
},
_flushReads: function(){
this._flush(this._reads);
this._waitsForRead = false;
},
_flushWrites: function(){
this._flush(this._reads);
this._isWriting = true;
this._flush(this._writes);
this._isWriting = false;
this._waitsForWrite = false;
},
};
window.fastdom = new Fastdom();
})();BTW: I don't need more features, than what you see in the script above. For me the simple writer helper is the most important one.
@aFarkas are you on IRC, perhaps we can chat? I'm wilsonpage on Mozilla and Freenode IRC.
The demo implementation looks interesting!
setImmediate doesn't really exist in the wild does it? I'm slightly concerned about using setTimeout as there's no guarantee when it will fire.
I thought about perhaps using microtask for reads, like Promise.resolve().then(...).
Disclaimer: I'm no longer working at the Financial Times where I was using FastDOM on a daily basis, so feel a little detached from the library and the common use-cases. This makes me cautious to introduce changes; but seems like you know what you're talking about :)
@wilsonpage
I made a short test with the promise approach and it failed (i.e.: It is called too early.) Wich means I also removed the setImmediate function and always use setTimeout. Calling setTimeout inside a rAF will always give you a time after the current frame but before next rAF so it is save. But even if not: The _flushWrites also calls the _flushReads so you get at least what you have now.
I just moved the script here, so we can discuss this better:
https://github.com/aFarkas/fastdom/blob/gh-pages/fastdom-discuss.js
But the promise approach could be used maybe here:
https://github.com/aFarkas/fastdom/blob/gh-pages/fastdom-discuss.js#L27
All this said, I will come into irc in 2-4h. Need to do stuff before.
I'm impressed with your level of analysis! I'm sure between us we can land a patch to make this better :)
I suggest any patches get landed on the v1-beta branch, when we're happy we can merge this into master and publish the long overdue v1.0 release.
Unrelated but related to reads: I have been wanting to play around with the idea of deferring reads till after 'load' to prevent premature reflow before natural first-paint. What are your thoughts on this?
OMG,i'm confused.................................
2015-10-12 16:21 GMT+08:00 Wilson Page notifications@github.com:
I'm impressed with your level of analysis! I'm sure between us we can land
a patch to make this better :)I suggest any patches get landed on the v1-beta
https://github.com/wilsonpage/fastdom/tree/v1-beta branch, when we're
happy we can merge this into master and publish the long overdue v1.0
release.Unrelated but related to reads: I have been wanting to play around with
the idea of deferring reads till after 'load' to prevent premature reflow
before natural first-paint. What are your thoughts on this?—
Reply to this email directly or view it on GitHub
#65 (comment).
Holding off developing this further following comments from @paullewis in this thread. Happy to re-consider once we have confirmation that DOM reads inside rAF is not preferable from one or two people close to Blink/Gecko engines.
My focus right now is shipping a solid v1.
@wilsonpage
Totally understand this. Maybe @jakearchibald has something to say. At least in this comment he says:
Ideally, treat your tasks as layout/style read only, and do your writes within raf.
Closing this until we decide to re-open.
Sorry to resurrect this. @aFarkas you mentioned this
b) reads should not happen inside a rAF (Animating the scrollTop/scrollLeft position is the only read, that should be tolerated)
What would you recommend if updating scrollTop/scrollLeft inside a rAF significantly slows down layout and painting, but doing the update after rAF causes a visible flash due to the changed scroll position? What would be the sweetspot for updating the scroll position to ensure that layout/painting is fast and the visible flash doesn't happen? I posted simplified code samples of what I'm currently doing in a separate issue for this so you can respond there if you prefer. Thanks!