View Issue Details

IDProjectCategoryView StatusLast Update
0018563MMW 5Generalpublic2022-05-01 17:19
Reporterdrakinite Assigned To 
PriorityurgentSeverityminorReproducibilityN/A
Status closedResolutionreopened 
Product Version5.0.2 
Target Version5.0.3Fixed in Version5.0.3 
Summary0018563: [Performance] Speed up SVG icon loading
DescriptionJH TODO: For performance reasons it might be useful to create a cache of DOM objects and use cloneNode() instead of innerHTML for copying to the live DOM.

While loading SVG icons is very fast to begin with, using cloneNode() is about 10 times faster still. To avoid breaking compatibility with code that requires the SVG code, we can add a loadIconFast() method that returns an already-created SVG icon.
TagsNo tags attached.
Fixed in build2600

Relationships

related to 0018800 closedmichal Keyboard navigation dies .. affects all panels, all tabs 

Activities

drakinite

2021-11-19 23:43

developer   ~0066009

Added in 5.0.3

michal

2021-11-23 14:15

developer   ~0066049

Last edited: 2021-11-24 12:23

This fix caused problems with displaying of some icons in several places. Problems found in:
Track Properties - Custom tab
Track Properties - Basic tab for Youtube tracks (icons next to Location)
Options - Collections & Views
Options - Auto-organize
Options - Media Sharing - Options... - Auto-conversion (and related Set Formats dialog)
Options - Layout - Preview - Advanced - Choose fields dialog on Setup button
Album view - Browser - Youtube tracks icons

drakinite

2021-11-24 19:47

developer   ~0066103

Fixed

Ludek

2021-11-26 12:11

developer   ~0066130

Last edited: 2021-11-26 12:13

I still see the double icon issue occisionaly, e.g. for a playlist in the playlist header (other playlists does not exhibits the problem). It's random.
image.png (4,048 bytes)   
image.png (4,048 bytes)   

Ludek

2021-11-26 17:54

developer   ~0066134

Last edited: 2021-11-26 18:01

And another regression I found is that when there is 'Grid (by Album)' view then the view icon button does not expand.
Not sure how it is related, but reverting the changes to this issue does the trick, to be found why this is happening...

EDIT: The change to loadIconFast in popupmenu.js is responsible for this, reverting the change fixes the issue with the menu not opened at all

Ludek

2021-11-26 18:24

developer   ~0066135

Last edited: 2021-11-26 18:25

Assigned to me to resolve these regressions asap.

Ludek

2021-11-26 18:48

developer   ~0066136

I fixed various further regressions that I found
Namely:
1) if svg failed to load (like in case of missing 'resize' icon) then the callback wasn't called at all (causing the view menu to not open at all)
2) fixed doubling the icon sometimes by introducing setIconFast(div, icon) that cleans the current content before setting the new icon and ensuring that the new icon exists

BTW: Are you sure that loadIconFast is actually faster than loadIcon ???
From what I am seeing in the loadIcon code there is already the caching available, see
var loadedData = loadedIconsHTML[iconName];

drakinite

2021-11-27 23:37

developer   ~0066143

Last edited: 2021-11-27 23:41

As discussed offline:

As for speed: Yes, I've tested and it is significantly faster. The performance improvement increases exponentially with SVG size/complexity - but even for the simple icons in the Material skins, cloneNode + appendChild is much faster than setting innerHTML. I believe it's because you set HTML, the engine has to do a decent amount more work parsing the string - while if you're cloning a node, it already knows its memory structure and just has to copy it. Similar in concept to how JSON.parse() on a long string is faster than an object literal.

Setting innerHTML = "" (from the recent fix) is very fast however, and completes in nanoseconds if the div was empty to begin with. (Microseconds if there is a little bit of HTML in the div).
The alternative [ while(div.lastChild) div.removeChild(div.lastChild); ] is notably slower than setting innerHTML = "". There's probably a special case in the native method for clearing a div via that method.

peke

2022-05-01 16:40

developer   ~0067902

Verified 2619

@drakinite do you think that we should document that special case so that can be considered for developers?

drakinite

2022-05-01 17:15

developer   ~0067903

Re Setting innerHTML = "" vs. while(div.lastChild) div.removeChild(div.lastChild): No, I do not believe this is necessary to document. I don't think the wiki should include weird details about JS best practices or how to maximize performance; it should focus on how to make addons for MediaMonkey. I'm still torn on 0018145 being included in the guide.

Re explanations on loadIconFast vs. loadIcon: Added to https://www.mediamonkey.com/wiki/Getting_Started_(Addons)#Icons