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

[Brainstorming] Better performance for updating selected #33

Open
pzuraq opened this issue Mar 14, 2021 · 0 comments
Open

[Brainstorming] Better performance for updating selected #33

pzuraq opened this issue Mar 14, 2021 · 0 comments
Labels
help wanted Extra attention is needed

Comments

@pzuraq
Copy link

pzuraq commented Mar 14, 2021

Not a standard feature request or bug report really, this is really something that would be very much a micro-tuning type of change, and I think it's something we should only consider doing if we happen to have a lot of spare time 😛 this is more of a braindump of some knowledge about the internals

Basically, the current solution for checking if an option is selected is a bit suboptimal. To be clear, it is definitely idiomatic, it's the way you would expect to do it:

{{#each @options as | optionValue |}}
  <option value={{optionValue}} selected={{is-equal optionValue @value}}>
    {{optionValue}}
  </option>
{{/each}}

But the issue is, we are checking is-equal inside each item in this loop. That means that whenever @value changes, we need to re-iterate every option in the loop to check if it is selected again. I've been thinking about a way to prevent this at the VM level, but so far I haven't been able to come up with anything. The state is being used and consumed in each item, and for all it knows, each item could be selected (I guess in a multiple select, it actually could!)

This just seems a bit unfortunate. Iterating an array in JS is really really fast, but if you go through a few layers like we have to when rendering, it gets much slower.

I really don't have any ideas about how we could do this better necessarily, maybe we can't! I think wrapping the options in objects with a @tracked selected property would actually be bad, because then we'd be creating and allocating more objects which would hurt initial render, and would increase overall memory usage.

One idea, which is definitely a bit out there, would be to use a modifier. This modifier could manually read the options and selected values synchronously and then update the <select>'s state accordingly. I really am unsure about this approach though, I'd definitely want to benchmark it to see if it actually works, and I'd want to think about whether or not it makes sense in general from a rendering perspective.

Anyways, it's possible the current solution is the optimal one, so I think for now it should stay the way it is. I'm just throwing this out there as something I've been thinking about 😄

@hergaiety hergaiety added the help wanted Extra attention is needed label Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants