wilsonpage/fastdom

Perf issue when using in loop to get width/height & set to other element [Update: Invalid issue]

manikantag opened this issue · 8 comments

manikantag commented

UPDATE: My original tests are incorrect or at least not the actual scenarios fastdom is meant for.
I originally read the height/width only once and used the same values in loop. So, clearly there is not need of fastdom here.

I've updated my test code to simulate both DOM reads and writes in loop, and very clearly fastdom heloped a LOT. See my comment about fixed tests.

So, this is an invalid issue, and fastdom clearly helping

Below is the original issue I've created:

Hi,

Currently I'm working on a custom CSS rendering engine for a specific need and I'll be querying offsetHeight/offsetWidth frequently and will be setting style.height/style.width and other style properties of elements. I'm thinking to use fastdome to see if I can get any perf benefits, but it seems other way.

I've created gist with and without fastdom: https://gist.github.com/manikantag/630288fb67453bb88c2493f8bd46aadd
(Note: I'm using fastdom-promisified with async/await).

Use below links to preview directly:

With fastdom: https://gist.githack.com/manikantag/630288fb67453bb88c2493f8bd46aadd/raw/fastdom-async.html

image

Without fastdom: https://gist.githack.com/manikantag/630288fb67453bb88c2493f8bd46aadd/raw/no-fastdom-test.html

image

When using fastdom, I'm seeing several callstacks in Chrome performance audit, which is not seen when not using fastdom.

Am I missing anything here? I expected completely opposite behavior!

manikantag commented

Ok. I think async/await is the culprit here. I've tried with just callbacks and Promises. Both of them are way better than async/await (while callback version performed little better than Promise version)

Callback version: https://gist.githack.com/manikantag/630288fb67453bb88c2493f8bd46aadd/raw/fastdom-callbacks.html

image

Promise version: https://gist.githack.com/manikantag/630288fb67453bb88c2493f8bd46aadd/raw/fastdom-promise.html

image

wilsonpage commented
manikantag commented

Thanks for confirming about Promise part (so thus async/await, which is performing way bad... at least now).

I still see the code without fastdom is performing (little) better. Is this because my example is a trivial one? Are there any specific real-world-scenarios where fastdom will benefit my usecase?

wilsonpage commented

I originally wrote fastdom to allow 'black box' UI components to read/write to the global dom without clashing via a shared scheduling layer (fastdom). If you are in full control of your code and don't need to share the space with third-parties/black-boxes, then you will probably squeeze better perf by rolling your own bespoke solution.

manikantag commented

That clarifies better. Thanks @wilsonpage

manikantag commented

@wilsonpage Excuse me for my bad repro page. The tests I've done all WRONG. I'm just reading the offsetHeight/offsetWidth only once and using them for all the 1000 elements created in the loop.

I've created new test pages which both reads and sets the DOM props in loop, which is the general scenario in most cases. And I can clearly see the benefits of fastdom.

Without fastdom (correct test): https://gist.githack.com/manikantag/630288fb67453bb88c2493f8bd46aadd/raw/no-fastdom-correct-test.html
image

With fastdom (correct test): https://gist.githack.com/manikantag/630288fb67453bb88c2493f8bd46aadd/raw/fastdom-callbacks-correct-test.html
image

fastdom is clearly the WINNER. It has brought down the Rendering from 233ms to just 30ms. EXCELLENT!

Again, excuse me for my bad tests and confusing you :) Thanks again for excellent work.

  • 👍2
aFarkas commented

@manikantag

Just some note on your promisified example. It is basically "slow" because you wait inside the mutation loop for each element to be added before you queue the next dom change. At this time fastdom thinks that you are already outside of the current mutation phase and queues your DOM change for the next one. This means every element is added in another rAF and not the same rAF as you want to.

The following two examples are simply not equal.

for (let i = 0; i < 10; i++) {
    // every queue is immediately processed
    readPromise().then(() => {
        // read
    });

    writePromise().then(() => {
        // write
    });
}

for (let i = 0; i < 10; i++) {
    // if i = 1 the readPromise waits for writePromise of i = 0 to be finished before executed.
    await readPromise();
    // read

    await writePromise();
    // write
}

So the main problem of your first example is not that you are using promises but that your async/await construct is not executed in the way you wanted to write your code in the first place.

  • 👍1
manikantag commented

@aFarkas You are correct. Thanks for pointing that.