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

add glimmer #3

Merged
merged 17 commits into from
Nov 27, 2018
Merged

add glimmer #3

merged 17 commits into from
Nov 27, 2018

Conversation

lifeart
Copy link
Contributor

@lifeart lifeart commented Nov 22, 2018

image

  • this is not real results (see comments)

@lifeart lifeart changed the title [WIP] add glimmer add glimmer Nov 22, 2018
@NullVoxPopuli
Copy link
Contributor

Exciting!

@jdaviderb
Copy link

good 👍

@somebee
Copy link
Owner

somebee commented Nov 23, 2018

Color me impressed :D I can't wait to dig in to how you are able to achieve this. I know we Glimmer and Imba are using similar ideas, but I'm having a hard time understanding how it is even possible to improve by this much. Looking forward to look at how we can improve Imba even further.

  1. A few things though, there seems to be something wrong with clicking to edit a todo - it does not receive focus, and is not cancelled when clicking elsewhere.

  2. I see that Glimmer is doing many more dom mutations than the others (another sign that the real dom is far from the real bottleneck today :P). Are you using keyed lists? That is, if a todo element moves, is glimmer rearranging the items or just overwriting from the top?

@somebee
Copy link
Owner

somebee commented Nov 23, 2018

In raw synchronous and blocking reconciliation with no state changes, glimmer seems to be about twice as fast as Imba at this point. Running 100k iterations (in the context of each implementation):

console.time('t'); for(var i = 0; i < 100000; i++){ API.forceReconcile(); }; console.timeEnd('t');

Imba takes around 120ms and glimmer around 55ms. Incredible work by the team behind Glimmer! I was thinking that Imba was approaching the theoretical limit since we practically do no unneeded operations per iteration, but looks like there is a lot to gain by making the compiled rendering less object-oriented and be even more careful about simplifying the stack etc.

@lifeart
Copy link
Contributor Author

lifeart commented Nov 23, 2018

@somebee https://www.youtube.com/watch?v=nXCSloXZ-wc

@lifeart
Copy link
Contributor Author

lifeart commented Nov 23, 2018

@somebee focus issue solved. And I think Imba can use glimmer-vm for rendering, it will help both communities.

@OEvgeny
Copy link

OEvgeny commented Nov 23, 2018

According to the top screenshot, Glimmer doesn't calculate left items count correctly.

@lifeart
Copy link
Contributor Author

lifeart commented Nov 23, 2018

@OEvgeny it's already fixed, you can try ureself testing this PR 65f151a

@yawboakye
Copy link

@somebee Is this PR ever going to get merged?

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

Hey. I'm sorry for the delay. As I've stated before, I've been incredibly impressed by the performance of glimmer. After digging in I do have some questions regarding the implementation though;

As I have stated before, the whole point of this benchmark is to test raw dom reconciliation performance. There is a reason we are not using shouldComponentUpdate in react (nor shouldRender in Imba), and that we are strictly requiring to use the state from a shared implementation, which uses mutable data structures. This is not fair to the performance of any of the frameworks as a whole. But it is fair to test the performance of the actual dom reconciler. How fast can we bring a view into sync with a state we do not know beforehand? Many frameworks are so intertwined with state management that this type of benchmark is not applicable to them. See comment by judofyr

I see from the implementation that Glimmer seems to not really use the data from the shared api, it clones it on every tick (setState) and lets glimmer do some sort of magical state tracking itself? Is there a way to force glimmer to synchronously run the actual dom reconciliation without knowing beforehand what data has changed?

I see that you also trigger app.scheduleRerender(); when something changes. Is this fully synchronous? Can I arbitrarily change the properties inside the mutable API state, then call some method that will guaranteed result in the dom being updated to reflect the state, synchronously? And if so, is that what is happening here?

Btw, could you use the actual methods from API to toggleTodo, renameTodo, removeTodo etc? I'm really not trying to be nitpicky here, but since people are already critical of the benchmark and don't really understand what we are trying to test, I'd hate to accept an implementation that circumvents the whole thing.

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

https://github.com/glimmerjs/glimmer.js/blob/master/packages/%40glimmer/application/src/application.ts#L230 According to this scheduleRerender is asynchronous, which defeats the purpose. this._rerender is what should be called.

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

Also, do not use the gzipped size, no other implementation is using that. Or, I guess all of them should probably change to gzipped.

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

Just added a test now, and the implementation is not synchronous. Will push the test in a few.

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

When this is run synchronously it's 27 times slower than Imba 1.3.3.

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

@somebee, regarding "Just added a test now, and the implementation is not synchronous. Will push the test in a few." can you do add "start/stop" trackers into you api, to patch sheduleRerender method?

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

@somebee Also, do not use the gzipped size, no other implementation is using that. Or, I guess all of them should probably change to gzipped. - agree, lets use gzipped sized for all items

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

Do you see that it becomes awfully skewed when the benchmark is doing tons of iterations synchronously to measure speed with a certain confidence, while the glimmer implementation is simply not doing any sort of reconciliation here?

"Glimmer use tracked properties to detect state changes" That is fine. Vue does this as well, React and Imba can do that using mobx or similar. The whole point of this benchmark is exactly to test the performance of the part that takes an (unknown) state and brings the view in sync as quickly as possible. The real test of Glimmer DOM reconciliation performance is to not track properties, and use the data from shared state directly, then call app._rerender() at the same place where the other implementations are doing that.

There are many other benchmarks that are testing the "real performance" of apps using special state management etc, but the whole premise here is to show that it is possible to make the actual dom reconciliation so fast that you can drop state management and property tracking all together.

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

I see two alternatives, not sure which you would prefer:

  1. Update the PR to remove the tracking and use state directly - (and call _rerender in sync like others).
  2. Just call _rerender inside API.render and remove setPropertyDidChange? As I said, API.render is only called after each step. The issue with this alternative is that Glimmer will still spend some time cloning and tracking data in setState that it ends up not using.

@NullVoxPopuli
Copy link
Contributor

@somebee I'm a little confused about the purpose of this benchmark now.

  • is it - to just test the speed of initial render where state hasn't come in to play yet?
    • if so, it would make sense for all libraries to remove ALL state management, including the memoized dom that I think the blog on this benchmark was about
  • something else?
    • I am unclear.

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

@somebee thank you so much for your time and this findings!

Found this woraround glimmer sandbox going to update this PR soon

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

@NullVoxPopuli No, the purpose is almost the opposite; to test the speed of reconciling when a very small part of the state has changed, but where the dom reconciler does not know what part beforehand.

I'm not saying it is not nice to have automated tracking of dependencies like glimmer, mobx etc. We could use the exact same techniques with Imba (and react, and the others), but then we wouldn't really test dom reconciliation. If actual dom reconciliation is fast enough, we don't need this.

And yes, all state management is basically removed in these implementations. State is managed by a small shared implementation which mutates the objects in the simplest manner possible. Btw, it is fine if glimmmer wants to render only after it sees that some properties have changed, but it has to do it synchronously in the same runLoop - otherwise the only part of Glimmer that is tested here is how quickly it can set two attributes in setState.

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Nov 26, 2018

he purpose is almost the opposite; to test the speed of reconciling when a very small part of the state has changed, but where the dom reconciler does not know what part beforehand.

Then I guess maybe I am lacking some understanding of something fundamental:

  • what is a "dom reconciler" this is a new term to me with all the imba blog / posts.
  • does every library have one?
  • if every library does not have one, what's wrong with the internal state tracking?
  • what is the difference between a dom reconciler and how other libraries manage state / track updates?

If actual dom reconciliation is fast enough, we don't need this.

just sounds like you'd befit more from having tracked state :p

Anywho, looking forward to @lifeart's updates!

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

@somebee can we add more clearness into test?
can you add to API something like API.state = JSON.parse(JSON.stringify(API.state)); before API.render call to prevent state leaking? (some libraries, including glimmer define own props on objects under symbolized property)

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

I think glimmer will be lower on this due to state mapping on each rerender, but it will be equal in context of this benchmark.

slower - because (at least) glimmer will patch new objects before each render.

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

@lifeart Does glimmer have any way to just reconcile? That is, can I write an app using glimmer, change some property on an object (that is rendered somewhere), and then make sure Glimmer reflects this change in the dom? We can definitely clone the full state on each render, but will that not make Glimmer even slower?

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

@somebee just reconcile - nope, it's KVO, so there is only few ways to keep DOM & state sync:

1.) Grab all state objects, invalidate changed props and ask about rerender. (tricky for arrays and require some logic).
2.) Set absolutely new object and ask about rerender (it will bind KVO on render).

Can I write an app using glimmer, change some property on an object (that is rendered somewhere), and then make sure Glimmer reflects this change in the dom? - yes, you need to notify builtin KVO tracker about change -

import Component, { tagForProperty } from '@glimmer/component';

function rerender(ctx) {
  const owner = ctx.__owner__;
  owner._rendering = true;
  owner._rerender();
  owner._rendering = false;
  owner._scheduled = false;
}

function markAsDirty(ctx, prop) {
  tagForProperty(ctx,  prop).inner.dirty();
}


export default class extends Component {
  count = 1;
  increment() {
    this.count++;
    markAsDirty(this, 'count');
    rerender(this);
  }
}

I think we can get something closer to real-life example, using kinda notifyPropertyChange from API, for example, if state.tasks[0].title changed, API can callback like notifyExternalAppAboutChange('tasks','0','title') where arguments - path to property in state.

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

Ok, so then I guess it's enough to change the whole state by cloning via JSON before rendering. My main point here was at least to not schedule rendering for a different runLoop, because that actually moves all the work outside of the benchmark. Doing the same with Imba would result in the same speed - all we are testing is the performance of API.step() which mutates som data. Your existing example but just calling app._rerender() from API.render() should do the trick - or am I misunderstanding something? You should obviously ask it not to scheduleRerender from setPropertyDidChange. If you wanted to, you could even do like this:

import { ComponentManager, setPropertyDidChange } from '@glimmer/component';
import App from './main';

const app = new App();
const containerElement = document.getElementById('app');

var shouldRender = false;
setPropertyDidChange(() => {
  shouldRender = true;
  // app.scheduleRerender();
});

app.registerInitializer({
  initialize(registry) {
    registry.register(`component-manager:/${app.rootName}/component-managers/main`, ComponentManager);
  }
});

app.renderComponent('Glimmer', containerElement, null);

app.boot();

setTimeout(()=>{
  if (typeof API !== 'undefined') {
    API.render = function(){ 
      if (window.appComponent) {
        window.appComponent.setState(API.store);
        if(shouldRender){
          app._rerender();
          shouldRender = false;
        }
      }
    };
    API.READY = true;
    API.reset(6);
  }
}, 10);

This would not trigger render more than once per setState, and it would call render in the same runLoop so that it is actually measured by the benchmark. I can make these changes myself and merge it in if you want me to :)

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

@somebee so close, but we need to drop all @tracked from app internals to prevent "after rendering", I think I can do it today/tomorrow morning.

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

I thought glimmer was mostly a self-contained library to very efficiently apply updates to the DOM. Does it even help to remove @tracked if Glimmer actually needs to track everything to allow you to rerender the view?

I see that if it is not state management agnostic in any way, and essentially relying on all data being handled through it's own KVO thing, this benchmark will not show the true performance of Glimmer (when you accept to use glimmer for data layer as well).

At least, it has to do things synchronously in the same runLoop as the benchmark :)

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

@somebee we'll see xD.

was mostly a self-contained library to very efficiently apply updates to the DOM yeah, kinda, but also, can be used like Alternative View Layer for an Elm App

I think any correct results will be interesting for both communities and core team, including running in same runLoop example.

I don't want to confuse anybody in JS/VUE/ANGULAR/REACT/EMBER/* community with uncorrect bench example.

@NullVoxPopuli
Copy link
Contributor

second @lifeart's concerns. with how much finagling is needed to make this bench ok, it seems like it could mislead people in passing (it already mislead me a bit)

@somebee
Copy link
Owner

somebee commented Nov 26, 2018

I kind of agree. I was pretty mislead myself - ended up spending hours to improve the performance of Imba reconciliation by over 30% before I discovered that the Glimmer benchmark didn't measure anything inside the glimmer implementation at all :P

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

image

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

@somebee can you please test aganist latest commit?

@lifeart
Copy link
Contributor Author

lifeart commented Nov 26, 2018

@somebee can you review this pr?

@somebee
Copy link
Owner

somebee commented Nov 27, 2018

This is now looking very complex in the sense that it tries to work around how Glimmer really works in a bunch of places. I'm not sure I understand why it would not be okay to use the simpler approach I suggested in #3 (comment).

Also, as @chilicoder mentioned on Twitter, Glimmer is doing a lot more dom mutations than the other implementations. Since all implementations are using the same mutable state it's definitely not the fault of the benchmark, but I'm curious to understand why it happens. Is glimmer basically recreating the dom for all the todo items even if nothing has changed?

Happy to accept it now, as it is syncronous, but I guess it's up to the glimmer community. It's not really showcasing the strengths of glimmer, since we enforce implementations to essentially not use KVO (we are enforcing a simple mutable state). It would be fun to see a idiomatic glimmer implementation as well tough -> I suspect it would still be slower than Imba, which has no KVO, nor a virtual dom, and simply reconciles the whole view every tick :)

@NullVoxPopuli
Copy link
Contributor

It would be fun to see a idiomatic glimmer implementation as well tough

#7 ??? :D

@lifeart
Copy link
Contributor Author

lifeart commented Nov 27, 2018

@NullVoxPopuli this is not optimized implementation xD.
@somebee regarding your comment - current approach is more maintainable xD.

I hope we can comeback soon with other implementations.

Happy to accept it now - good news! Thanks!

@NullVoxPopuli
Copy link
Contributor

this is not optimized implementation xD.

right, but it'd be cool if we had a benchmark with all the same measurements, but showcased each view library in its entirety, utilizing all the little performance tricks each has to offer

@somebee somebee merged commit 29041f8 into somebee:master Nov 27, 2018
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.

6 participants