luwes / js-diff-benchmark

Simple benchmark for testing your DOM diffing algorithm.
66 stars 5 forks source link

Is there a Replace Test? #21

Closed ryansolid closed 4 years ago

ryansolid commented 4 years ago

Replace is slightly different than clear and insert as it has to first realize that it can't keep anything that's there. I thought I saw it before maybe? Was it removed?

WebReflection commented 4 years ago

The replace operation is literally a remove and insert though, as that's what the DOM tree needs to do.

I'd be ok counting replace as one operation, cause performance/numbers bake the fact it costs similarly anyway.

ryansolid commented 4 years ago

I guess I was thinking if you pass an empty array you could optimize that more than an array where nothing is kept. So the clear portion isnt representative of the clearing needed for replace. As most of the cost is in the DOM maybe doesnt matter but feels like it could.

WebReflection commented 4 years ago

I think I was talking about the replaceChild operation, not the replace all rows one, which I guess is what you are after. In that case, the update every 10th row replace 100 up to 1000 nodes, so it should be a good enough indication of a replacement.

However, the replace all is a non interesting use-case, imho, as the reason we're all using diffing is because we all want to avoid such operation, and nothing can compete with a node.textContent = "" followed by a node.innerHTML = allNodes, and the best way to replace all is to clear and append, as that's surely a 2k operations.

It could be 1k instead, via replaceChild done thousand times, but we count replaceChild indeed as two operations already, as it's a remove, and insert one.

That being said, if you really think that brings any value, it seems trivial to implement.

ryansolid commented 4 years ago

However, the replace all is a non interesting use-case, imho, as the reason we're all using diffing is because we all want to avoid such operation, and nothing can compete with a node.textContent = "" followed by a node.innerHTML = allNodes, and the best way to replace all is to clear and append, as that's surely a 2k operations

Right, it is the worst case scenario which seems precisely why to test it. The point of testing it is no library is going to know it is in this case unless it checks unlike being given both operations separately. If they all behave similar no big deal, but if one does use textContent = "" because if it detects that state, that's fair game. How much extra code did it cost? Does it affect the iteration in general?

I think it's also super common place in declarative libraries using pagination. While it would be more efficient to clear the list perhaps (using textContent="") before loading the next page. Generally you just set the page data. It does no favours for the reconciler, but it's what you do. So much so its hardly an edge case.

WebReflection commented 4 years ago

Yeah, pagination is actually a very good example, I just fear I need to scroll the console horizontally to see results 😅

Will propose a test

WebReflection commented 4 years ago

P S. using textContent is not an option for udomdiff or in case the before is used, as that would break the rest of the container layout, so it'd be great to see which library does that, and which one is safe.

ryansolid commented 4 years ago

I think Stage0 does. Atleast my mod of it did before I switched to udomdiff. But that is fair, the before case is different and can never leverage it anyway.

WebReflection commented 4 years ago

all libraries seem to work fine without destroying the extra content when before is passed along.