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

Re-evaluate usage of TextTruncator and optimize its performance #8124

Closed
MisRob opened this issue May 28, 2021 · 11 comments
Closed

Re-evaluate usage of TextTruncator and optimize its performance #8124

MisRob opened this issue May 28, 2021 · 11 comments
Labels
P2 - normal Priority: Nice to have

Comments

@MisRob
Copy link
Member

MisRob commented May 28, 2021

Observed behavior

TextTruncator triggers layout reflows that on pages with many instances of TextTruncator can cause significant performance issues, as observed in #7525.

This is the first part of the profile (at some point my laptop runs out of resources and doesn't record anymore) of Learn section of Ciencia Espacial topic containing 1151 sub-topics (which means 1151 of TextTruncator instances) of Ciencia NASA channel where the following bottlenecks can be observed:

profile

As can be seen from the following, for a page containing hundreds of TextTruncator instances, there is an enormous number of expensive style calculations and layouts/reflows run.

Performance analysis for a single TextTruncator instance:

(1) TextTruncator -> mounted -> ResizeSensor (KResponsiveElementMixin) -> attachResizeEvent

Screenshot from 2021-05-27 10-28-55

1 style recalculation 1 layout/reflow
Screenshot from 2021-05-27 10-29-13 Screenshot from 2021-05-27 10-29-37

(2) TextTruncator -> ResizeSensor (KResponsiveElementMixin) -> resetExpandShrink

Screenshot from 2021-05-27 10-33-33

1 style recalculation 1 layout/reflow
Screenshot from 2021-05-27 10-34-03 Screenshot from 2021-05-27 10-34-13

(3) TextTruncator -> handleUpload -> shave

Screenshot from 2021-05-27 10-40-14

around 6 style recalculations around 6 layouts/reflows
Screenshot from 2021-05-27 11-08-29 Screenshot from 2021-05-27 11-08-54

Currently, we useTextTruncator in the following components and can expect similar performance issues on all pages utilizing them:

  • LessonContentCard
  • ChannelCard
  • ContentCard

Related KResponsiveElementMixin background by @indirectlylit

For some context, the responsive element mixin (and resize sensor) were originally an attempt to emulate element queries. The intent was to better support responsive behaviors for complex layouts where it’s the available space determined by the layout (as opposed to the overall window size) which should influence internal responsive behaviors of a child component.In practice though, I think we rarely use it for that reason. A more common use I’ve seen of the resize sensor is for smart text-eliding behaviors. For example, I think the breadcrumb component uses a responsive element to drive rendering details.


Let's re-evaluate what are our requirements for text truncation in Kolibri while taking these performance issues into account:

  • Is the usage of TextTruncator build around KResponsiveElementMixin and shave for so many instances of the component on one page valid?
  • Do we need so granular information about layout updates all the time?
  • Can we avoid using ResizeSensor in some situations?

Based on the outcome of the discussion, do we need to ask KDS team for any updates of KResponsiveElementMixin?

This issue could set a precedent for recommended usage of KResponsiveElementMixin elsewhere that will be together with performance warnings ideally documented in KDS.

Errors and logs

Source performance profile (can be loaded in Chrome)

Expected behavior

The components mentioned above are performance-wise optimized

User-facing consequences

  • Slow loading and navigation or complete unresponsiveness
  • Unnecessary device battery drain (? not an expert)

Steps to reproduce

  • Import Ciencia NASA channel
  • Navigate to Learn -> Channels -> Ciencia NASA -> Ciencia Espacial

Context

Kolibri 0.15.0.dev0+git.20210520221349
Chrome
Ubuntu

@MisRob
Copy link
Member Author

MisRob commented May 28, 2021

During researching this problem a doing some study I noticed the following techniques, that might be helpful if we decide to optimize. For example, for shave (3rd party library right now), this could reduce 6 layouts into 1:

Batch DOM changes and perform them "offline". Offline means not in the live DOM tree. You can:

  • use a documentFragment to hold temp changes,
  • clone the node you're about to update, work on the copy, then swap the original with the updated clone
  • hide the element with display: none (1 reflow, repaint), add 100 changes, restore the display (another reflow, repaint). This way you trade 2 reflows for potentially a hundred

Source: Rendering: repaint, reflow/relayout, restyle

I am not saying that we need to implement our own shave, it depends on the outcome of the discussion regarding our usage of TextTruncator. However I find it to be interesting, and taking into account Kolibri use-cases, it might be good to invest some time into optimizations even when we end up just with a couple of ResizeSensor and shave instances on a page.

Resources that I've found to be helpful:
CSS Triggers - overview of CSS changes that trigger layout/reflow and paint
Rendering: repaint, reflow/relayout, restyle - rather older, but accessible and clear summary demonstrated on examples
Reduce the Scope and Complexity of Style Calculations - up-to-date summary including recommended techniques related to style calculations
Avoid Large, Complex Layouts and Layout Thrashing up-to-date summary including recommended techniques related to layout/reflow

And one interesting library that seems to be often recommended in relation to performance problems: FastDOM

FastDom works as a regulatory layer between your app/library and the DOM. By batching DOM access we avoid unnecessary document reflows and dramatically speed up layout performance.

@MisRob
Copy link
Member Author

MisRob commented May 28, 2021

Also see #7525 (comment) and #7525 (comment):

@rtibbles Could we delay initialization of resizeSensor until after the initial page load in some way?
@indirectlylit That is exactly what I was thinking - there's no reason the resize sensor start operating until the initial page load has settled. There's also a fair chance this has been causing less drastic performance issues throughout the app, so this would be a nice change everywhere

and #7525 (comment)

@jonboiser One possible way to address this is to put the whole list in a virtual window (using something like vue-virtual-scroll-list) which would probably fix the initial 30 second component initialization (which is already long) and possibly some of the other things.

@MisRob
Copy link
Member Author

MisRob commented May 28, 2021

Regarding

@rtibbles Could we delay initialization of resizeSensor until after the initial page load in some way?
@indirectlylit That is exactly what I was thinking - there's no reason the resize sensor start operating until the initial page load has settled. There's also a fair chance this has been causing less drastic performance issues throughout the app, so this would be a nice change everywhere

Does anyone have any idea how to detect the initial page load reliably in a SPA? I experimented a bit with Vue lifecycle hooks and $nextTick but so far haven't discovered something that would significantly help.

@rtibbles
Copy link
Member

Were the measurements on TextTruncator taken before or after this PR was merged, btw? #8101

@rtibbles
Copy link
Member

Does anyone have any idea how to detect the initial page load reliably in a SPA? I experimented a bit with Vue lifecycle hooks and $nextTick but so far haven't discovered something that would significantly help.

I think the basic mechanism we would probably have to use is that the SPA would need to explicitly report when it was done with initial page load, and then turn the resize sensor on after that. I don't think there would be a good way to do it systematically.

The only other thing I can think of would be to do some sort of debounce across VueJS $nextTick intervals, and take it that if say 5 $nextTick intervals passed without any more changes, then you let the resize sensor trigger.

@MisRob
Copy link
Member Author

MisRob commented May 31, 2021

Were the measurements on TextTruncator taken before or after this PR was merged, btw? #8101

I have #8101 commits locally on a branch I worked on so it's possible that it was taken rather after, but I can't say for sure, because I fetched develop more times and took more recordings. It will be good to add a last commit hash information to performance recording next time.

@MisRob
Copy link
Member Author

MisRob commented May 31, 2021

I think the basic mechanism we would probably have to use is that the SPA would need to explicitly report when it was done with initial page load

@rtibbles Are you referring to load event by this? That might help when reloading a page (to some extent - when playing with performance profiles for reload, it looked that many bottlenecks are happening after load) but not when navigating from one page to another within one SPA, especially when the latter is a problematic page (like in #7525, where you at first arrive in Learn section, where load occurs, and then navigate to Ciencia NASA / Ciencia Espacial from there.

The only other thing I can think of would be to do some sort of debounce across VueJS $nextTick intervals, and take it that if say 5 $nextTick intervals passed without any more changes, then you let the resize sensor trigger.

Sounds interesting. It might be also good to have some fallback logic to make sure that resize sensor load won't be postponed forever, just in case there is some logic on a page that is regularly making changes. For example, to say that if it wasn't loaded after 20 seconds, load it no matter what's going in in $nextTicks. I am not sure if there are such pages in Kolibri though, but maybe it won't harm anything in any case.

@MisRob
Copy link
Member Author

MisRob commented May 31, 2021

Another idea regarding detection of the right moment to start the resize sensor - what about playing with PerformanceObserver's entry for Largest Contentful Paint, at least for browsers that support PerformanceObserver? I haven't worked with it yet and am not sure if it could be easily used for this purpose, just sounds like an interesting way to explore to me.

@MisRob
Copy link
Member Author

MisRob commented May 31, 2021

In any case, before we dive into particular optimizations, it would be good to discuss some high-level questions raised in the description. I don't know what were the reasons for implementing TextTruncator in this way and if it's okay to simplify it, or reduce its usage, or if there are good reasons to continue using it as is.

@indirectlylit
Copy link
Contributor

indirectlylit commented Sep 21, 2021

For some context on the original motivations – TextTruncator was added in #3508 to satisfy a few goals:

  • Set a fixed maximum height on a block of arbitrary-length text, with width responsive
  • Maximize use of allocated screen real estate by filling in bottom line of text completely
  • Only segment between full words
  • Use a real ellipsis character that will be read correctly by screen readers, and have it located correctly relative to the last visible word

image

@MisRob
Copy link
Member Author

MisRob commented Oct 2, 2023

Closed by #8532

@MisRob MisRob closed this as completed Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 - normal Priority: Nice to have
Projects
None yet
Development

No branches or pull requests

3 participants