-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Comments
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 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> 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. |
Cheers for the quick response!
|
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 😄 |
Thanks for the follow up. There are two conversations here imo,
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 |
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.
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. |
🙏🏻
…On Sat, 7 Mar 2020 at 15:22, Chris Garrett ***@***.***> wrote:
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.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#18798?email_source=notifications&email_token=AAC5I3N3BKVCJJ7KKVBWFYTRGJRD3A5CNFSM4LDH42Z2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOD4BRY#issuecomment-596099271>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAC5I3P7MRN2YSMR2P33ZG3RGJRD3ANCNFSM4LDH42ZQ>
.
|
Is the issue specific to Chrome with the devtools open or is it always the same relative speed (console open or not)? |
As per video, it seems specific to Chrome with the dev tools open. I can’t explain this. |
This is also what I've noticed when debugging |
…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
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:
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.
The text was updated successfully, but these errors were encountered: