Skip to content

Standardize implementations #757

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

Closed
kaushalyap opened this issue Jun 26, 2020 · 7 comments
Closed

Standardize implementations #757

kaushalyap opened this issue Jun 26, 2020 · 7 comments

Comments

@kaushalyap
Copy link

kaushalyap commented Jun 26, 2020

Please define a implementation standard and ask contributors to follow the standard. To understand what I mean please read this comment.

@ryansolid
Copy link
Contributor

ryansolid commented Jun 26, 2020

It's misleading. Many virtual dom libraries do implicit event delegation. So while React looks like it is attaching per row it isn't. It actually attaches a single event handler to the document.body and does its own propagation. Same with Inferno, Ivi, Solid, DomC, Stage0 etc.. I think even Elm does this too. So basically many other libraries did it explicitly to match. VanillaJS to properly serve as a reference needs to use fastest techniques any libraries use.

There is some debate whether if it isn't part of the library should they be doing it. But personally fair is fair from my perspective. If these libraries are attaching a single event listener why can't any library do so?

EDIT: The greater shame is libraries with only a non-keyed implementation.

@kaushalyap
Copy link
Author

kaushalyap commented Jun 26, 2020

@ryansolid So in summary you mean current comparison is fair?

@ryansolid
Copy link
Contributor

ryansolid commented Jun 26, 2020

Not all implementations are equivalent. But given that event delegation is built in to many libraries we can't say one could not implement that way.

But conversely some implementors feel it isn't idiomatic. I made an explicit delegated version of Svelte but never got support from the community so I withdrew my PR. Svelte values code and bundle size very highly.

There is a whole thread about this and similar concerns #430 . That thread and linked threads can give you a good idea.

So in summary the current comparison is not fair but if the implementors and library authors chose it could be, but there is as much value in them being able to represent their library in the way that feels right for them.

@localvoid
Copy link
Contributor

@kaushalyap don't take this benchmark seriously, the only useful information that you can get from this benchmark is that libraries on the right side of the table don't have fast paths for test cases that are used in this benchmark, everything else is useless and misleading. A lot of "fast" libraries in this benchmark actually pretty bad when they can't handle some use case with a fast path. And the real reason why many libraries are using explicit event delegation is because they have expensive data bindings.

@ryansolid
Copy link
Contributor

@localvoid That's a pretty pessimistic view.

But back to the OP's question. Would you say it is fair that libraries not use explicit event delegation when many libraries do implicit event delegation? Whether a library has implicit event delegation has nothing to do with expensive data bindings. Every library takes a hit in a benchmark involving thousands of rows if they don't delegate events.

I'm not going to claim all implementations are equivalent. But the only test that actually exposes fast paths for reconciliation is test5 swap rows as it's the only one that actually moves rows around. Most libraries fall into 2 categories ones with naive swap all elements in the middle and ones with fast paths. There are better swapping algorithms but most libraries that do those also do fast paths. test4 I agree is complete mess in terms of standardization and should be taken with a grain of salt. It has different implementations, variance issues, and so on.

The rest of the tests are simple. And more or less show what they show. Based on batch creation or more granular updates they definitely benefit certain libraries. But that's sort of the point? All benchmarks should be looked at with suspicion. But that isn't specific to here.

@localvoid
Copy link
Contributor

Would you say it is fair that libraries not use explicit event delegation when many libraries do implicit event delegation?

I am not arguing about fairness, it is insanely hard to implement a fair benchmark.

But the only test that actually exposes fast paths for reconciliation is test5 swap rows as it's the only one that actually moves rows around.

Pretty much every test exposes fast paths in one way or another. Lets take for example your library and the datastructure that you use for edges. When observable has 1 edge, the fast path is to preallocate it with exactly one slot, but the 2nd edge will increase internal array buffer by 16 slots, so it will have 15*2 useless slots (120bytes with v8 pointer compression, 240 without). With an API that uses call-by-name evaluation strategy at abstraction boundaries and property-based observables, observable graph in real apps won't be so simple like in this benchmark, and memory overhead will be quite different.

And please don't take it as a personal attack, I even don't want to discuss all this stuff, as it is not the point that I want to make :) It is a good decision to implement it this way, the problem is that because tests in this benchmarks are so simple, it doesn't show the big picture. A lot of different primitives require different tradeoffs, especially abstraction boundaries, as it is the most complicated problem when we are talking about performance. Abstraction boundaries change everything, no matter what algorithms are used for a dataflow.

@krausest
Copy link
Owner

I'm closing this issue. We will not standardize the implementations.

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

4 participants