search

PolymerElements / iron-iconset-svg

Represents a source of icons expressed as a collection of inline SVGs
https://www.webcomponents.org/element/PolymerElements/iron-iconset-svg
37 stars 34 forks source link

Addresses a perf regression related to mirroring #59

Closed cdata closed 7 years ago

cdata commented 7 years ago

The original implementation computed direction specific to the subtree where the icon is to be stamped. This is the most correct approach, but also incurs some non-trivial cost that can add up quickly (getComputedStyle must be called for every mirrored icon).

The implementation has been changed so that direction is only computed once per page load per iconset when a mirrored icon is stamped. This should reduce the number of getComputedStyle calls to 1 in most scenarios.

bicknellr commented 7 years ago

Since applyIcon/removeIcon's targets are each individual icon, the RTL-ness needs to be tested and recorded on a per element basis. Probably also worth adding a note to the docs for those methods so that people aren't confused if they try to move the element with the applied icon between areas with different alignment and find that their icon isn't getting flipped back.

bicknellr commented 7 years ago

(from offline discussion) It's known that this prevents iron-iconset-svg's use in mixed writing direction pages. The justification for this correctness -> performance tradeoff is that the feature this is intended to fix was introduced very recently and is too costly due to synchronous style read thrashing. Also, _targetIsRTL doesn't have much of a purpose anymore and was introduced with the mirroring feature, so it should probably be removed.

Here's a thought: if applyIcon didn't immediately flip the icon but queued them all to be checked in a rAF callback, there would only be one style read per set of inserted icons even if they were being created in a tree with other elements performing style writes in between. That way, we'd still be able to have mixed writing modes. This doesn't particularly help if the page containing the icons is already being created in pieces split over other timeout-like callbacks, since each chunk would cause a new rAF. However, if the PR is intended to fix the large, single, synchronous parse and create situation that you might see when initially constructing a page, then this might work.

cdata commented 7 years ago

I added some documentation to the methods affected by this change.

Regarding this rAF suggestion: this might be feasible. However, it opens up the door for potential pathological conditions. For example, not all icons are stamped up front, and if there is a condition where (for example) a list of comments is paging in while a user scrolls and getting icons stamped into them asynchronously, this could be pretty bad.

bicknellr commented 7 years ago

a list of comments is paging in

Yeah, no benefit to the rAF approach in this case. :/

LGTM then; _targetIsRTL still sounds strange given the memoization but I don't really have a better name in mind.

(from offline discussion) If both the writing direction of the page is known and the iconset is accessible at the same time, they could set rtl-mirroring at that point and we wouldn't need to test at all if rtl-mirroring meant always flip icons with mirror-in-rtl. (Maybe even rename these to mirroring / mirror since they wouldn't really be based on writing direction anymore?)