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

V3 #7

Merged
merged 14 commits into from
Aug 6, 2017
Merged

V3 #7

merged 14 commits into from
Aug 6, 2017

Conversation

joshgillies
Copy link
Owner

@joshgillies joshgillies commented Jun 29, 2017

Massive refactor, opting for a Class based API as default. Still need to write more docs, but overall I'm happy with where the API is now.

Will ping the following issues and aim to resolve them before closing this PR off.

  • Add documentation Update docs #5
  • Add tests Add tests #3
  • Implement API for managing Component Composition Composability via component nesting #8
  • Review requirement for HyperComponent.prototype.adopt - may drop this as it's still marked as experimental in hyperHTML anyway Provide an adopt API? #9
  • Lock in APIs for the following:
    • Component props - might followReact here and simply use constructor(props) as the standard way for setting this.props.
    • Component state - as above, the React model of managing local state via setState() may be ideal.

@joshgillies
Copy link
Owner Author

@nodefish - I've just updated the above to reflect work that should ideally be completed before cutting a release. What do you think?

@nodefish
Copy link

nodefish commented Jul 7, 2017

@joshgillies Apologies for the delay in responding (timezone difference). The above looks good to me!

@joshgillies
Copy link
Owner Author

@nodefish no problems, thanks for taking the time. 👍 I'll continue working on the above and keep you posted on progress.

@nodefish
Copy link

nodefish commented Jul 7, 2017

Any way I can contribute? Would gladly work on this if needed.

@joshgillies
Copy link
Owner Author

@nodefish right now if you're able to have a look over the examples (shamelessly ripped from the React website) in the README.md, and have a play with the APIs that are there that would be amazing.

@nodefish
Copy link

nodefish commented Jul 8, 2017

I've messed around with it for quite a while now, and figured I'd try doing an implementation for the popular js-framework-benchmark (https://github.com/krausest/js-framework-benchmark).

Usability/API wise, it's very promising. It's a good experience once the quirks with things like this.html and this.wire are understood. I think we can ultimately abstract those away so end users don't have to deal with them much.

However, at the moment there are some pretty serious performance problems when handling large lists. It's unclear to me at this time where the bottleneck is, but generally speaking, using the following pattern is slow in lists of ~10k items compared to other frameworks:

render(){
    const rows = this.data.map(d => this.wire(d) `<div>${d.id}</div>`;
    return this.html `
                    <div>
                        (...stuff)
                    </div>
                    <div>${rows}</div>`;
}

It's (strangely) ~20% faster compared to the above situation if we use Component list items rather than raw html tagged with this.wire, e.g.:

// Parent class: "Main"
render(){
    const rows = this.data.map(d => this.child(DataRow, { key: d.id, parent: this, store: this.state.store, data: d }));

    return this.html `<div>${rows}</div>`;
}
/*......*/

// Child class "DataRow"
render(){
    return this.html `<div class="row">${this.props.data.label}</div>`;
}

...but it's still very slow overall compared to other frameworks. Other frameworks are slow upon the first render, but afterwards they use reordering rather than re-rendering the entire list. I thought wire was supposed to be good at this, but wiring 10k items doesn't seem to work well. I'm not sure at all where the bottleneck is.

@joshgillies
Copy link
Owner Author

@nodefish - nice work! I had intended on using https://github.com/krausest/js-framework-benchmark as a point of reference. Are you able to share the code you're working with?

@nodefish
Copy link

nodefish commented Jul 9, 2017

Here it is: https://github.com/nodefish/hypercomponent-js-framework-benchmark

Note that it's a bit "quick and dirty" as I've been doing some unversioned experimentation (a couple of the tests may have broken because of this, the select test in particular is a bit flaky and I may have broken it).

It seems the bottleneck is due to the fact that the benchmark trashes and rebuilds the list from scratch upon each iteration. Other frameworks are able to cope with this because of their VDOM diffing, whether keyed (reordering) or unkeyed (reuse existing DOM elements and overwrite their attributes indiscriminately). I expected this.wire to handle this, but it does not seem to. I've included some of my wire code commented out in the render function of Main.js. I erased the other experiments for the time being.

FYI the fastest way to work with this is to run npm start in the root directory, and then navigate to http://localhost:8080/hypercomponent-v3 rather than running the entire benchmark (which is incredibly time consuming even for 1 framework). It prints performance timings to the console so you can get a quick idea of how badly it's performing simply by clicking on the buttons and observing the time. I recommend opening the hypercomponent implementation and one other in another tab (say, inferno or preact) and compare.

@joshgillies joshgillies force-pushed the v3 branch 10 times, most recently from 7463ae3 to 42ad0e7 Compare August 5, 2017 07:51
@joshgillies joshgillies force-pushed the v3 branch 3 times, most recently from e2091a0 to 7fce0ba Compare August 5, 2017 16:14
@joshgillies
Copy link
Owner Author

Oh, I've finally got around to getting this branch in a position where I'm personally happy to cut a release.

@nodefish I've reduced the API surface area quite a bit since we last spoke. If you're able to take another look that would be appreciated. Though I'll likely cut a release in a few hours as I'd like to have v3 public sooner than later.

In addition I've looked into adding hypercomponent and also hyperHTML into the https://github.com/krausest/js-framework-benchmark suite. And aim to focus on performance in the coming weeks.

@joshgillies joshgillies force-pushed the v3 branch 3 times, most recently from 6d9f737 to 2ee02e8 Compare August 6, 2017 00:07
@joshgillies
Copy link
Owner Author

v3 has been released! 🎉

@nodefish
Copy link

nodefish commented Aug 6, 2017

Hey @joshgillies, I'll definitely take a look soon. Nice job!

re:benchmark: unless I'm missing something fundamental about how wires work (which is possible since I've been busy with life for a while and lost track of this a bit), the way this benchmark works seems really unfavorable for HyperHTML because it trashes and recreates the entire list on each iteration, including recreating all the wires if you use a wire approach. With VDOM, the diffing algorithm doesn't care if the list objects are new or not, as long as the values are the same; but for HyperHTML, wires are by reference, so I believe it re-renders everything on each iteration. At least that's my operating assumption on why it's slow.

Will attempt to get involved again in this soon and review ways to overcome this. It's probably possible to nullify this issue by persisting the wires in an array, as seen in the DBMonster test but I don't know if that's within the rules of the benchmark. This would be like a wire equivalent of what is being done with the component tracking its children already (this.children), assuming that bit remained in the v3 that you just released.

@nodefish
Copy link

nodefish commented Oct 12, 2017

Hi @joshgillies

I'm aware it is now several months later. I actually had to disappear due to health reasons, which are now resolved. Replying because I wanted to keep my word about checking it out. I actually did take a look at your work back in August and I like the direction you've taken the project. 👍

I must however say I'm still stumped by the performance profile of the HyperHTML backend in large lists.

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

Successfully merging this pull request may close these issues.

2 participants