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

Performance issue #60

Open
mindplay-dk opened this issue Nov 4, 2019 · 7 comments
Open

Performance issue #60

mindplay-dk opened this issue Nov 4, 2019 · 7 comments

Comments

@mindplay-dk
Copy link

I noticed that the setState callback currently updates not only the affected node, but all of it's siblings.

render(vnodes, dom), // And then we call-out to the re-render function

State updates are synchronous, so imagine you have to update 10 sibling components - each component calls setState and triggers updates of all the other component, so a total of 10*10 = 100 updates.

Since the function signature is render(vnodes, dom), it looks like there's currently no way to update an individual child node.

Likely this needs to be refactored, something like extracting a function renderNode(vnode, domParent, domChild) and calling that from the setState closure instead?

@wavesoft
Copy link
Owner

wavesoft commented Nov 4, 2019

This is unfortunately true...

I started doing some benchmarks with js-framework-benchmark and noticed a particularly bad behaviour on bulk updates. That's the same issue as you observed.

Indeed, some serious refactoring will be required, since this will definitely exceed the size budget 😞

@mindplay-dk
Copy link
Author

...this will definitely exceed the size budget 😞

Okay, so, just a thought, but I was able to remove all of this:

dot-dom/src/dotdom.js

Lines 76 to 95 in ddd6d50

let wrapClassProxy = factoryFn =>
new Proxy( // We are creating a proxy object for every tag in
// order to be able to customize the class name
// via a shorthand call.
factoryFn,
{
get: (targetFn, className, _instance) =>
wrapClassProxy(
(...args) => (
(_instance=targetFn(...args)) // We first create the Virtual DOM instance by
// calling the wrapped factory function
.a.className = (_instance.a.className || " ") // And then we assign the class name,
+ " " + className, // concatenating to the previous value
_instance // And finally we return the instance
)
)
}
);

And this:

dot-dom/src/dotdom.js

Lines 272 to 284 in ddd6d50

window.H = new Proxy(
createElement,
{
get: (targetFn, tagName) =>
createElement[tagName] || // Make sure we don"t override any native
// property or method from the base function
wrapClassProxy( // Otherwise, for every tag we extract a
createElement.bind(createElement, tagName) // class-wrapped crateElement method, bound to the
) // tag named as the property requested. We are not
// using "this", therefore we are using any reference
} // that could lead on reduced code footprint.
)

All without affecting the core functionality - which makes me think, the core library itself has no dependency on this, and JSX works fine without out, so does this have to be part of the core library?

It seems you could push this to a plugin and give yourself a little more elbow room, so we don't trade-off performance for a few extra bytes saved?

@wavesoft
Copy link
Owner

wavesoft commented Nov 4, 2019

Yes, you are right. This is just implementing the Declarative DOM Composition extension and is not part of the core library.

I haven't gone through the benchmarks yet, so I haven't really compared the final results of JSX versus declarative DOM. If JSX is a clean winner on the final size, I will consider removing it.

However so far this:

div.a("Hello")

Compresses more efficiently than this:

H("div", {className: "a"}, "Hello")

But on an actual project, the repetitive H("div",{className: " chunks could be nicely compressed, so the outcome is not clear.

@wavesoft
Copy link
Owner

wavesoft commented Nov 4, 2019

But yea, if this allows us to achieve better reconciliation performance, I will consider it.

@mindplay-dk
Copy link
Author

I'd expect the declarative DOM syntax would compress better, but someone (me) might still choose JSX perhaps mostly out of preference - so it would be nice if this API was optional either way.

I mostly suggested it as a work-around to your painstaking 512 byte limit 😁

@mindplay-dk
Copy link
Author

This may be slightly off-topic, but here's my unminified version so far, if you're interested. I dropped the proxy stuff and turned it into an ES6 module. Currently clocks in at 519 bytes, I'm sure that's completely unacceptable to you. 😜

@Jemt
Copy link

Jemt commented Jul 26, 2020

It would be interesting to see how .dom stack up against other frameworks. I have previously used https://github.com/mathieuancelin/js-repaint-perfs (online demo here) myself to test a simple templating engine I built.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants