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

A lot of empty text nodes - I mean A LOT - performance issue #3586

Open
neuronetio opened this issue Sep 18, 2019 · 16 comments · May be fixed by #3642
Open

A lot of empty text nodes - I mean A LOT - performance issue #3586

neuronetio opened this issue Sep 18, 2019 · 16 comments · May be fixed by #3642
Labels
awaiting submitter needs a reproduction, or clarification feature request perf stale-bot

Comments

@neuronetio
Copy link

Describe the bug
Just check this repl https://svelte.dev/repl/902d4ccfcc5745d0b927b6db6be4f441?version=3.12.1

There are 22 000 empty text nodes that are really slowing down entire component on update.

obraz

Expected behavior
I know that those nodes are for a reason but maybe some virtual approach will be better?
Each node must be created updated and destroyed which takes a lot of time and memory when something change.

Severity
This behavior can lead users to leave svelte because they might not figure out what is wrong - code seems to be ok, but performance is really bad (22 000 extra nodes for 2 500 real ones :O ).
For me this is annoying because I must be careful and watch for those "extra" nodes that are appearing randomly in some circumstances.

@daliusd
Copy link

daliusd commented Sep 18, 2019

For something like this you should consider using https://github.com/sveltejs/svelte-virtual-list . Virtual list of course will not work for this specific problem, but that should give an idea.

@neuronetio
Copy link
Author

Yeah, but I have a grid in my component with a lot of cells and those cells needs to be visible all the time - no scrollbar. REPL above is just simple example :)

neuronetio added a commit to neuronetio/svelte that referenced this issue Oct 1, 2019
@neuronetio neuronetio linked a pull request Oct 1, 2019 that will close this issue
4 tasks
@Rich-Harris
Copy link
Member

@neuronetio can you share any numbers on the performance before/after #3642? we actually used comment nodes previously, but changed it to empty text nodes because it's less cluttered when you open devtools. If there's a noticeable performance impact then I guess we probably do need to revert it, though this does seem like an extreme case 😀. Maybe it could even be an option...

@marcus-sa
Copy link

marcus-sa commented Oct 10, 2019

@Rich-Harris comment nodes are way better for this usage.
There's a reason Angular is using them.

@daliusd

This comment has been minimized.

@shirotech
Copy link

+1 for comment nodes, also related #4423

@Conduitry
Copy link
Member

Does anyone have any benchmarks for empty text nodes vs comment nodes?

@shirotech
Copy link

@Conduitry https://jsperf.com/empty-createtextnode-vs-createcomment/1

Comment nodes are slightly faster, but regardless of that comment nodes I think is more correct because it doesn't influence dom structure or styling/layout.

@gonchuki
Copy link

I think the discussion about text vs comment nodes derails the main issue at hand here which is the large amount of superfluous nodes being created.

Any slight difference between text vs comment nodes can shift around between browser engines or different versions of the same engine, but the creation of nodes that shouldn't be there is guaranteed to degrade performance in all environments.

@shirotech
Copy link

@Conduitry what would it take for comment nodes to be considered? Right now I'm just using a fork, as the app actually crashes with text nodes in IE11 if it was used in a WordPress environment (their emoji replacing function).

@antony
Copy link
Member

antony commented Apr 9, 2020

Strange that those nodes only appear in Firefox and not in Chrome.

@shirotech
Copy link

@antony They are in Chrome also, just hidden from the Elements tab of inspector tool. They still exist in the DOM.

@jcschmidig
Copy link

Even worse, the emtpy text nodes are styled by the browser (width and height) which leads to unpredictable effects.

@AlbertMarashi
Copy link

AlbertMarashi commented Apr 2, 2021

Even if there are improvements that could be made to the engine, it's probably a good idea to use comments.

I could see this causing issues in page builders that merge/remove text blocks, and text nodes are styled by the browser which is probably why the performance issue is occurring in the first place.

@stale
Copy link

stale bot commented Dec 23, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@legoheld
Copy link

One usecase where the empty text node fails is in contenteditable nodes.
Especially in Firefox the empty text nodes will be rendered as extra whitespace and confuse the selection.

More precisely I built a slate view similar to: https://nathanfaucett.github.io/svelte-slate/default
And there the textNodes make the selection and rendering completely useless in FF.

When I adjust the dom.js like this:

/**
 * @returns {Text} */
export function empty() {
	return comment('');
}

It works without any issues. Therefore I would prefer comment nodes instead of text nodes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting submitter needs a reproduction, or clarification feature request perf stale-bot
Projects
None yet