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

Chrome slowness #18798

Closed
amk221 opened this issue Mar 6, 2020 · 9 comments · Fixed by #18861
Closed

Chrome slowness #18798

amk221 opened this issue Mar 6, 2020 · 9 comments · Fixed by #18861

Comments

@amk221
Copy link

amk221 commented Mar 6, 2020

I've created a simple app that renders 500 glimmer components in a loop. Mousing over one sets it as active.

Here is a quick video demoing it: https://www.youtube.com/watch?v=ySuUKGfpoGw

There are 3 parts to the video:

  • development environment with the dev tools open.
  • development environment
  • production environment

As you can see, with the dev tools open, it's barely usable (and this makes day to day development very frustrating!)

I understand that there are assertions stripped in production builds, which is why it is faster. But, I do not expect to see such a large difference with the dev tools open.

@pzuraq
Copy link
Contributor

pzuraq commented Mar 6, 2020

Has this problem gotten worse recently? In 3.17, we added the ability for Glimmer VM to have DEBUG assertions for the first time. These allow us to have some really valuable errors around correctness of certain core features, but they could be regressing performance in development.

Beyond that, it looks like the fundamental problem here is the architecture of these components. The way that it works now, by passing down activeChild as an argument to the child component and having the child component check whether or not it is the active component means every time you change that value, all 500 components will rerender. That's going to be costly no matter what.

Screen Shot 2020-03-06 at 1 25 30 PM

I would recommend managing the active/hover status within the component directly, since hover states change very quickly. Ideally, you would use plain CSS to manage this state, since that'll be the most efficient method for doing it, but a local setting is also pretty fast

import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import { action } from '@ember/object';

export default class TestChildComponent extends Component {
  @tracked
  hovered = false;

  @action
  toggleHover() {
    this.hovered = !this.hovered;
  }
}
<div class="test-component__child
  {{if this.hovered "test-component__child--active"}}"
  {{on "mouseenter" this.toggleHover}}
  {{on "mouseleave" this.toggleHover}}
>
  {{yield}}
</div>

Screen Shot 2020-03-06 at 1 34 26 PM

For actually selecting the value, you'll still need to pass the selected state down of course. That's a much less common action, so it shouldn't be as bad if it's not performant, but if it is there is one way you can optimize it currently:

<div class="test-component" {{on "click" this.toggle}}>
  {{#if this.show}}
    {{#each @options as |option|}}
      <TestComponent::Child @isActive={{eq option this.activeChild}} @onActivate={{fn this.activateChild option}}>
        {{yield option}}
      </TestComponent::Child>
    {{/each}}
  {{/if}}
</div>

By doing this equality check outside of the component, it means that the component will not be entered when rerendering, since its arguments and local state have not changed. This is currently slower than it needs to be (I have ideas for how to make it faster) but it will always be a little bit faster, since it doesn't need to do any further checks.

@amk221
Copy link
Author

amk221 commented Mar 6, 2020

Cheers for the quick response!

  • I don't think it's anything to do with glimmer components, because it's the same when switching to ember components

  • I don't think it's got slower recently, but rather gradually over time.

  • You are right, it can be optimised as per your example. But, I am reluctant to change the existing architecture because having the children compute their state automatically based on autotracked arguments is a taught pattern. It feels right, and keeps the code in the right place - I don't feel like I should have to refactor to optimise this sort of code. Also, this is a stripped down example. In a real app, there are more things to be computed per-child.

  • Architecture aside, there does seem to be something particularly bad with chrome dev tools open, compared to other browsers. (fwiw without ember inspector)

@pzuraq
Copy link
Contributor

pzuraq commented Mar 6, 2020

You are right, it can be optimised as per your example. But, I am reluctant to change the existing architecture because having the children compute their state automatically based on arguments is a taught pattern. It feels right, and keeps the code in the right place - I don't feel like I should have to refactor to optimise this sort of code. Also, this is a stripped down example. In a real app, there are more things to be computed per-child.

Personally, I disagree here. I think that using per-component state is the right choice for this particular problem, and it feels much better to me. I realize it may be a shift in thinking, but I think the current solution is somewhat fighting the reactivity model, and that's going to cause things to get even more difficult as time goes on.

This is similar to when people started moving away from observers. There were a lot of patterns that "felt right" using observers too, and it took quite a while and a lot of learning new solutions before that started to change in the community. I know it took a long time for me personally, I really liked observers at the time 😄

@amk221
Copy link
Author

amk221 commented Mar 7, 2020

Thanks for the follow up.

There are two conversations here imo,

  1. Slowness specific to Chrome
  2. My architecture.

I don't want 2. to detract from 1. But...

I can't pass down the selected state as you suggest, because unlike the stripped down example above, the actual component is closed over.

{{! parent.hbs }}
{{yield (hash child=(component "test-component" isSelected=(eq ???)))}}

I don't know the @value of child (to compare with its parent's @value. Because there is no instance at this point in time.

@pzuraq
Copy link
Contributor

pzuraq commented Mar 7, 2020

There are two conversations here imo,

  1. Slowness specific to Chrome
  2. My architecture.

Yes, and I hope I haven't sounded dismissive. I agree there is an issue with dev-time performance in Chrome and it sounds like we may have regressed recently. Unfortunately these types of problems can be difficult to track down, especially when they are browser specific, but when I have time (or someone else does) we'll try to deep dive and see if we can figure out ways to improve it 😄

The architecture convo is absolutely orthogonal, to me it's just about improving best practices.

I can't pass down the selected state as you suggest, because unlike the stripped down example above, the actual component is closed over.

Makes sense. I could imagine passing down an option ID or something that you pass back up when selecting, but that does seem to get into more complicated territory where it may not be worth it.

I think the more important bit is to not be doing this type of update based on hover, since hover can happen many times per second. Selection can be a bit slower and probably not be super noticeable, like I said before.

@amk221
Copy link
Author

amk221 commented Mar 7, 2020 via email

@rwjblue
Copy link
Member

rwjblue commented Mar 16, 2020

Is the issue specific to Chrome with the devtools open or is it always the same relative speed (console open or not)?

@amk221
Copy link
Author

amk221 commented Mar 16, 2020

As per video, it seems specific to Chrome with the dev tools open. I can’t explain this.

@pzuraq
Copy link
Contributor

pzuraq commented Mar 16, 2020

This is also what I've noticed when debugging

rwjblue added a commit to rwjblue/ember.js that referenced this issue Apr 3, 2020
…formance.

Removes capturing stack traces for all tracked items, as just calling
`new Error()` ends up taking quite a bit of time.

Fixes emberjs#18834
Fixes emberjs#18798
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 a pull request may close this issue.

3 participants