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

Reduce number of DOM manipulations to improve performance. #194

Merged

Conversation

jcass77
Copy link
Member

@jcass77 jcass77 commented Apr 27, 2016

I've tested these optimisations on tracklists of around 2 000 tracks. It basically just replaces the individual DOM manipulations on each item in the tracklist with one large append for the entire table.

Browsing tracks before optimisation:
orig-browse

Browsing tracks after optimisation:
opt-browse

Adding all tracks to Queue using PLAY_ALL before:
orig-play_all

After:
opt-play_all_1

@jcass77
Copy link
Member Author

jcass77 commented Apr 27, 2016

This PR is fairly straightforward and does not change any actual functionality. I think I'll merge it - let me know if it causes anybody any problems.

@jcass77 jcass77 merged commit 64eda54 into pimusicbox:develop Apr 27, 2016
@kingosticks
Copy link
Member

Out of interest, would you expect the 'graphics' time to double after the "browsing tracks" change? I.e is this real and will it affect the initial render time (which is arguably most important) or is this because the performance numbers are generally inaccurate?

@jcass77
Copy link
Member Author

jcass77 commented Apr 27, 2016

I'm not sure how accurate these are as I just performed a single benchmark as a sanity check. It is possible that the timings for come categories will increase as the browser will now try to render a bigger list at once, instead of incrementally.

I suspect this will differ between browsers and platforms.

In my experience, rendering the table piecemeal did not improve usability. Even though it might start rendering sooner, all of the DOM 'churn' that is happening behind the scenes still meant that the UI would not respond to user interaction. So you could not do much more than wait for everything to finish loading anyway.

It definitely feels faster with the changes applied.

@kingosticks
Copy link
Member

Sure, it's not practically useful to render part of the page early but it's visual feedback that something is happening and gives the illusion of being more responsive and is generally preferable on the Web. We shouldn't be battling any data download times so we don't have the usual case here.

@jcass77
Copy link
Member Author

jcass77 commented Apr 27, 2016

Thinking about it some more: all of the non-function-call-related timings really depend on how long you keep the benchmark running for. So I might just have stopped the benchmark recording for one run before the other.

There would still have been requests to last.fm for all of the album covers etc. So timings that are not directly related to the function calls or DOM manipulation are probably not comparable between runs.

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.

None yet

2 participants