Open lincolnthree opened 3 years ago
Thanks for the issue. Can you provide a GitHub repo that shows the performance issue as it is right now?
@liamdebeasi This issue isn't about the performance issue. It's more about conditional rendering. Trying to keep them separate, but wanted to explain some of the motivation for this. This one is an issue even if you just want to conditionally render things inside sliding item options. But I'll see what I can do when I get a minute.
Thanks for the follow up. I guess I am confused as the first post calls out performance as a motivation for this issue in the first paragraph, so I am trying to understand how these issues appear in applications.
@liamdebeasi Fair, it is performance motivated - that was the initial reason why I implemented this workaround a few months back - and am now revisiting it as I'm trying to address major perf issues on mobile devices. I'd certainly love to just shove all the elements on screen at once and have it work out. But the issue is still a problem when you want to add/remove things from ion-item-options
elements conditionally. I understand the need for a repro and will do my best.
Also not intending to be... err... mansplaining, I guess? I know you're an expert in this stuff. Just trying to explain a bit what I think is going on.
Counting the DOM:
ion-item-sliding
: 1 to 3 (depending if you have no options, or options on both sides). This one is lean.ion-item
: 11 elements just for slots and item-inner/wrapperion-item-option
/ion-button
: 15 at a minimum, each.Each <ion-button>
element added to a sliding item can result in upwards of 15-20 DOM elements *per ion-item-option
.
So adding this up. We have close to 30 DOM elements just to show the skeleton of an ion-item-sliding
with one option. This is OK so far, well within reason.
Now let's make it 5 options (not uncommon). Now each item requires ~90 DOM elements. Now add your own content to the item. Call it 100 to account for a few ion-label
s and ion-icon
s in the item itself, maybe a '...' button at the end.
Let's say we have a list of 100 items. At 100 elements per item, that means 10,000 DOM elements that need to be created all at once just to show the initial render.
On a mobile device that can be crushing. 7500 of these elements could be conditionally rendered only as needed - if item options allowed it. 100 items in a list is not uncommon, and shouldn't require virtual scrolling. Even 500 list items should be... sort of reasonable, you could incrementally load it. But that's just my current line of thinking of why this is a perf issue.
@liamdebeasi PS. Not sure if I'm missing it or if it's still a thing, but I don't think the stackblitz repos have been updated for Ionic 5.
@liamdebeasi I updated the repro with a reload button to make things a bit more visible. Feel free to play with the length of the list, I just set it at 500, which is slow even on a very beefy desktop. Even 100 takes a noticeable time to render. Then remove the options and try again.
And just for emphasis. I actually am surprised the DOM node count is this high: A list of 500 items.
@liamdebeasi Also just added a toggle for options on/off so you can more easily benchmark.
Putting this here because I don't trust StackBlitz:
<ion-app>
<ion-header>
<ion-toolbar>
<ion-buttons slot=start>
<ion-button slot=end fill=clear (click)="reload(true)">
<ion-toggle (ionChange)="toggle()" [checked]="optionsEnabled"></ion-toggle>
</ion-button>
</ion-buttons>
<ion-title>List of Sliding Items</ion-title>
<ion-buttons slot=end>
<ion-button slot=end fill=clear (click)="reload(true)">
<ion-icon name="refresh-outline"></ion-icon>
</ion-button>
</ion-buttons>
</ion-toolbar>
</ion-header>
<ion-content>
<ion-list>
<ion-item-sliding *ngFor="let i of items">
<ion-item-options side=start>
<ng-container *ngIf="optionsEnabled">
<ion-item-option slot=start color="primary">
<ion-icon slot=top name="chevron-back">
</ion-icon>
</ion-item-option>
<ion-item-option color="secondary">
<ion-icon slot=top name="chevron-forward">
</ion-icon>
</ion-item-option>
<ion-item-option color="tertiary">
<ion-icon slot=top name="chevron-up">
</ion-icon>
</ion-item-option>
</ng-container>
</ion-item-options>
<ion-item [button]=true>
<ion-label>{{i}}</ion-label>
<ion-button slot=end fill=clear>
<ion-icon name="ellipsis-vertical"></ion-icon>
</ion-button>
</ion-item>
<ion-item-options side=end>
<ng-container *ngIf="optionsEnabled">
<ion-item-option color="primary">
<ion-icon slot=top name="chevron-back">
</ion-icon>
</ion-item-option>
<ion-item-option color="secondary">
<ion-icon slot=top name="chevron-forward">
</ion-icon>
</ion-item-option>
</ng-container>
</ion-item-options>
</ion-item-sliding>
</ion-list>
</ion-content>
</ion-app>
import { ChangeDetectorRef, Component } from "@angular/core";
import { AlertController } from "@ionic/angular";
@Component({
selector: "my-app",
templateUrl: "./app.component.html",
styleUrls: ["./app.component.css"]
})
export class AppComponent {
optionsEnabled = true;
items = [];
constructor(private _cdr: ChangeDetectorRef) {
this._cdr.detach();
}
ngAfterViewInit() {
this.reload(true);
}
toggle() {
this.optionsEnabled = !this.optionsEnabled;
this._cdr.detectChanges();
}
reload(detect?: boolean) {
this.items = null;
if (detect) {
this._cdr.detectChanges();
}
window.setTimeout(() => {
this.items = new Array(500);
for (var i = 0; i < this.items.length; i++) {
this.items[i] = i;
}
if (detect) {
this._cdr.detectChanges();
}
});
}
}
Thanks for the additional information, that was very helpful. I can reproduce the behavior your are describing. I have not dug too much into this yet, but there seem to be a few factors that contribute to this performance issue:
ion-item-sliding
gesture and setup code: There is a lot of JavaScript that we currently run when creating an instance of ion-item-sliding
. In particular, code related to the gesture and computing values seem to be the most costly here. We do have plans to refactor this as part of https://github.com/ionic-team/ionic-framework/issues/22909, so that would be a good opportunity to address any performance issues there as well.ion-item
or ion-button
, but streamlining ion-item-options
and ion-item-sliding
is something we are definitely interested in.I will keep this issue open for now, but will re-evaluate when we get around to tackling https://github.com/ionic-team/ionic-framework/issues/22909.
PS. Not sure if I'm missing it or if it's still a thing, but I don't think the stackblitz repos have been updated for Ionic 5.
This is on our to-do list for our Framework v5.7.0 work, which is the next minor release of Framework v5: https://github.com/ionic-team/ionic-framework/milestone/101.
Forgot to add this, but does that pretty much cover the issues you are seeing? Is there anything I missed?
Thanks @liamdebeasi -- Partly. This mostly covers the performance related aspects regarding Stencil/Ionic - that part looks good.
However I am not sure it addresses the actual feature requested my original description, which is the ability to only render the contents of ion-item-options
when the item is opened on the respective side. I am also not sure it addresses the ability to add options/components while the slider is open/opening (properly resizing).
Regardless of how efficient you get the stencil code to be... we still either need conditional rendering of ion-item-options
or the ability to do it ourselves so that what we put into the slider can be handled efficiently as well.
https://user-images.githubusercontent.com/362329/113752293-c8c01300-96da-11eb-8926-85ad2234fb70.mov
I think this issue should probably be split up. One for the features I requested (this one or whichever you feel is most appropriate given this one really shifted toward perf), and one for the performance problems, which is what I attempted (and likely failed to do) by referencing the original performance issue: https://github.com/ionic-team/ionic-framework/issues/23116 -- does that make sense?
Assuming we resolve most of the outstanding performance issues with ion-item-sliding
, what would the use case for this feature be? As far as I am aware this kind of functionality does not exist in the native sliding component on iOS. When the sliding options are not revealed, we hide them using display: none
so there should not be many performance issues related to these.
I guess another way to think about this is where most of the performance issues are coming from ion-item-sliding
, being able to remove ion-item-option
elements from the DOM until they are ready to be shown likely will not make much of a difference. Another thing to consider is would manually removing/adding them to the DOM be creating more work for the browser as opposed to just leaving them in the DOM and hiding them with display: none
.
@liamdebeasi In general, the use case is I am putting something heavy into the ion-sliding-options
(not an ion-item-option
) that I don't want to render for each item unless the user interacts with that specific item.
Without the ability to defer this rendering, it will cause the same performance problems that you'll be spending time to fix. I am not familiar with the native iOS slider component, but you guys have the feature so we're using it :) Rendering 100 elements when a specific item is slid open is a lot better than rendering 10,000 elements when the whole list shows up. It's the same issue.
The only features we need to support this are more responsive sizing of the ion-item-options
in the sliders, and an event to let us know if the options are visible at all or not. I've implemented this myself using the (ionDrag)
event, but I still can't get the slider to respect the new size of the options. (Thus the need for a placeholder as I mentioned above.)
Also for the record, we are using this component for all platforms. Not just iOS.
<ng-container slot="options-end" *ngIf="state.editing">
<ng-container *ngIf="viewModel.options$ | push as options">
<ng-container *rxLet="cardItem.sliderOpen$ as sliderOpen">
<div *ngIf="!sliderOpen; else sliderOpenTemplate"
style="width: 185px">
</div>
<ng-template #sliderOpenTemplate>
<ion-item-option color="transparent"
(tap)="cardItem.toggleSlider()">
<ion-icon slot=top name="chevron-forward"></ion-icon>
</ion-item-option>
<ion-item-option color=secondary
[disabled]="!options.canMoveMain"
(tap)="actions.moveCard($event, deck, options.otherZone, 'main', card, 1)">
Main
<ion-icon slot=top name="arrow-back"></ion-icon>
</ion-item-option>
<ion-item-option color=secondary
[disabled]="!options.canMoveOther"
(tap)="actions.moveCard($event, deck, 'main', options.otherZone, card, 1)">
{{ options.otherZoneName }}
<ion-icon slot=top name="arrow-forward"></ion-icon>
</ion-item-option>
<ion-item-option (tap)="showCardMenu($event, deck, card)">
<ion-icon slot=top ios=ellipsis-horizontal
md=ellipsis-vertical>
</ion-icon>
</ion-item-option>
</ng-template>
</ng-container>
</ng-container>
</ng-container>
public slidingDisabledEnd$ = new BehaviorSubject(false);
public slidingDisabled$: Observable<boolean> = combineLatest([this.slidingDisabledStart$, this.slidingDisabledEnd$])
.pipe(map(([start, end]) => start && end));
private _sliderMoved$ = new BehaviorSubject(0);
private sliderMoved$ = this._sliderMoved$
.pipe(distinctUntilChanged())
.pipe(throttleTime(500, asyncScheduler, { leading: true, trailing: true }));
public sliderOpen$: Observable<boolean> = this.sliderMoved$
.pipe(switchMap(() => from(this.slider?.getSlidingRatio() ?? Promise.resolve(0))))
.pipe(debounce(ratio => ratio > 0 ? of(0) : interval(500)))
.pipe(map(ratio => ratio !== 0))
.pipe(shareReplay({ refCount: true, bufferSize: 1 }));
Regarding knowing which side is open, we already have a feature request open for this: https://github.com/ionic-team/ionic-framework/issues/18533
Sorry if this was already mentioned somewhere, but can you clarify what "responsive sizing" means in the context of ion-item-options
?
@liamdebeasi Ah yeah! Sorry, watch the video clip I attached above. It demonstrates adding an element while the slider is in motion. The size isn't correctly computed until the slider is opened, then closed, then reopened quickly (in my impl. because I have a debounce on the state change, see code snippit just above).
I guess another way to think about this is where most of the performance issues are coming from ion-item-sliding, being able to remove ion-item-option elements from the DOM until they are ready to be shown likely will not make much of a difference. Another thing to consider is would manually removing/adding them to the DOM be creating more work for the browser as opposed to just leaving them in the DOM and hiding them with display: none.
Maybe. I'm not 100% convinced. There's a lot of purple (rendering) in my benchmarks too. But I am not intimate enough with the browser rendering process to know the cost difference between display: none
and not rendered at all. If the JS performance-hit is slowing down rendering that much I could see that.
Regardless, let's assume there is a heavy Angular, Style, or other JavaScript cost to instantiating the user's components too. The core issue remains unchanged I believe. Maybe there are images in there, for example. That shouldn't be loaded until slid open... etc etc. Simply having images in the slider (think SVGs) would bottleneck the HTTP request performance for the browser as it resolves URLs.
PS. FWIW our users love the ability to slide items open. It's a great feature, heavily used even on Desktop.
@liamdebeasi In general though, when rendering many things -- assuming loops are the bane of performance -- isn't it best practice to render as little as possible until necessary (or give the option to), unless in specific instances where rendering batches of items in the same tick/frame prevents exploding layout recalcs?
Or am I missing something?
PS. This is a video of the whole hodgepodge put together, using a placeholder to force the width, and then fading the elements in, on first interaction:
I think it's pretty seamless and prevents pre-rendering every element under the sliding item :)
https://user-images.githubusercontent.com/362329/113757649-d9738780-96e0-11eb-98c8-f9cb12aa1e17.mov
OH. I forgot to mention another critical issue that really requires this feature... these items are transparent to support the theme background gradient :D
If you pre-render the contents of the item-sliding (or forget to set them to opacity: 0), it breaks everything because they bleed through. So you need to be able to toggle the opacity or something to that effect anyway. So that's yet another use-case for having the triggers feature you just linked. But it relates to this because this can easily done using dynamic rendering and solves two problems with one solution.
You could even start rendering the options when the user puts their finger on the item, before they even start a swipe, to prevent any appearance of "nothing here" rendering jank, if a fade is undesirable.
Ah yeah! Sorry, watch the video clip I attached above. It demonstrates adding an element while the slider is in motion. The size isn't correctly computed until the slider is opened, then closed, then reopened quickly (in my impl. because I have a debounce on the state change, see code snippit just above).
Ok that makes sense, thanks.
In general though, when rendering many things -- assuming loops are the bane of performance -- isn't it best practice to render as little as possible until necessary (or give the option to), unless in specific instances where rendering batches of items in the same tick/frame prevents exploding layout recalcs?
Yes however the biggest hits to performance will be rendering ion-item-sliding
given what we need to do to compute dimensions and get the gesture setup. Omitting a few buttons likely will not make a big difference. I guess what I am saying is if we address the main performance issues, removing these option buttons from the DOM ideally would not even be necessary and would just be creating more work for yourself. However, I won't be able to make that assessment until the initial performance issues are resolved.
We plan on refactoring the ion-item-sliding
component with desktop + performance improvements as part of our Framework v6 work which is already underway. I am not saying "no" to the other improvements noted in this thread, I am just saying that the team here needs to work on the performance improvements first before we can accurately determine what other work needs to happen. I will keep this issue open for now and will update this thread when we have completed work on the updates. Thanks!
@liamdebeasi That makes sense. And I hope you're right that it won't make a difference to have a "few" extra buttons. I'm just doing the math in my head and those buttons make a lot of nodes :) As long as you don't break what I've done ;) Haha! (I kid, I kid)
Looking forward to seeing what you can come up with. Until then, I hope this has been helpful at least. Your great work and valuable thoughts are always appreciated over here.
Agreed. I appreciate all the clarifying information you have given!
Oh one last thought regarding this:
the biggest hits to performance will be rendering ion-item-sliding given what we need to do to compute dimensions and get the gesture setup
Any way to defer gesture initialization/dimensions until the finger hits the element? (Obviously you need a listener of some kind to catch the initial & subsequent events) Just spitballing here. Could be the ultimate optimization but likely not doable, hah!
One actual last thought (hopefully) I'll leave here -- since this issue has kept me up for a few weeks and I just can't seem to get it off my mind (our primary value-driving screens use this feature extensively) and I'm having a hell of a time making things feel fast.
My real-world testing (and that stackblitz) really both demonstrate that rendering or deferring the ion-item-options
has a massive performance impact. Deferring the rendering of options speeds up the initial render of the list by over 2x, cuts the rendering time more than in half, in my testing (even on fast computers).
I really feel strongly that this should not be discounted as a general performance optimization (or at least be made more easily possible / optional as I've specified above in the issue desc, but, until I work for Ionic I don't expect to have more influence than what I've said here ;)
The effect is really hit home when you run the stackblitz on a low-end mobile device (Even a Google Pixel XL2 at this point suffers), which according to latest reports, mid-range devices now make up most of the mobile market. Flagships with fast CPUs will drop even more, by percentage.
I know I'm preaching to the choir again, but while it's true that the primary performance hit might be the javascript, sometimes taking the low hanging fruit can make the stuff at the top feel more attainable, and on these devices, every little bit matters a lot.
Carry on! Thanks again.
This recording shows just how much impact deferring those options has. Now, performance tuning of those options might be another story, but... in the current state, they seem to matter a lot.
https://user-images.githubusercontent.com/362329/113888530-6d505c80-9790-11eb-83ff-cd4b445172e0.mov
Hey @liamdebeasi, I know you're super busy - sorry to bug you, is the <ion-item-sliding>
refactor still on deck for V6? I have seen some of the gesture fixes come through. Thanks for those!
I do not know if this will make the 6.0 release, but we do still plan on doing this for Ionic 6. We are focused on getting the remaining Ionic 6 API changes finished and then we will work on bugs/performance improvements/etc.
I hope bumping this is OK since I'm thinking this may have gotten lost in the mix (no longer labeled for v6.x / part of the milestone). Just checking in. Feel free to ignore.
@lincolnthree Is the original performance issue still a problem with Ionic 7? Ionic Framework is focused on mobile apps, so the desktop changes I noted previously are not likely to happen. However, I wanted to see if the performance issues are still relevant so we can address them.
Thanks @liamdebeasi - We definitely do still have some perf issues with item-sliding, but I'll check the repro repo with V7. It's probably worth making sure the general issue is the same (or isn't caused by something else at this point.)
Sorry for the delay. On vacation and had to get to wifi.
Yes, there is still a significant difference in rendering performance here, though I can't tell you exactly what the reason is right now since I'm in the mountains and borrowing time and getting glares ;)
First two script execution timings on the timeline are "with sliding options enabled", and the last three are with sliding options disabled/not rendered:
Here is an updated branch/repro repo: https://github.com/lincolnthree/ionic-bugs/tree/issue-23155
Without digging farther, I can't tell you if the perf drop is due to the simple overhead of rendering more components, or if there's an issue here.
So there are 3 areas where performance is an issue in the sample:
Page Load
The app is trying to load 1000 components that each execute their own set of JavaScript. This is really more of a device performance issue than an Ionic bug. I'd recommend implementing virtual or infinite scrolling for this so your app does not try to load all the components at once. Lower end devices realistically are not going to be able to load and execute JS for all the components in a short amount of time.
Gesture Start
When the gesture starts I am seeing that Chrome has a lot of long "Task" items on the main thread. I need to take a closer look at why this is happening:
Gesture Drag
When trying to drag an item after the gesture starts, the gesture is almost unusable due to how sluggish it is. I did observe that the "Hit Test" in Chrome was taking a significant amount of time. I found that using content-visibility: auto
helped a lot. Can you try adding this to your sample and let me know if it helps improve the performance of the actual drag animation (once the gesture starts)?
ion-item-sliding {
content-visibility: auto;
}
Hey @liamdebeasi, Thanks. I was able to test this, and yes, the performance of dragging sliding items open was dramatically improved with this CSS. I was not even aware of the 'auto' mode for content-visibility
, so thanks for that as well. Looks like it's not particularly well-supported (at all on iOS), unfortunately.
I did test this on Safari on OSX and the dragging performance is indeed still an issue there.
I'd recommend implementing virtual or infinite scrolling for this so your app does not try to load all the components at once.
Yes, we do this where possible, but absent a quality virtual scroll solution that can support dynamic-height components, this isn't always feasible. We're generally only rendering 100 of these at a time in such scenarios, but that still impacts lower-performance mobile devices significantly.
You had mentioned previously that computing dimensions in the startup JS would be the biggest hit, but is there a way to share that work for elements that are rendered in the same parent container? I suppose not 100% reliably, since any element can be rendered with different styles. Maybe use a shared cache where the key is the sliding item dimensions? Not sure what computations are being done, just tossing out ideas.
I was not even aware of the 'auto' mode for content-visibility, so thanks for that as well. Looks like it's not particularly well-supported (at all on iOS), unfortunately.
Yeah, this would really be an Android-focused fix at the moment. Which iOS devices can you reproduce this performance issue on? I tested some of my older iOS devices, but the performance was pretty solid.
You had https://github.com/ionic-team/ionic-framework/issues/23155#issuecomment-814332664 that computing dimensions in the startup JS would be the biggest hit, but is there a way to share that work for elements that are rendered in the same parent container?
Off the top of my head I'm not sure, but it's pretty clear something needs to be improved in that realm so I can look into it a bit more.
Yeah, this would really be an Android-focused fix at the moment.
Android GPUs need as much help as they can get :(
Which iOS devices can you reproduce this performance issue on? I tested some of my older iOS devices, but the performance was pretty solid.
I tested on my M1 Max (maxed out) MacBook Pro. Though on closer inspection, I think the jank I'm seeing may have been mostly due to scrollbar shift (not sure why that's happening.) I might be imagining things though but I do see some jank while dragging as well. Not huge though.
https://github.com/ionic-team/ionic-framework/assets/362329/dd285282-0d7a-41ce-ae11-5c61cfd87b31
Yeah I believe that is due to the scroll bar. We disable scrolling when the gesture starts which is likely why the scrollbar disappears/the layout shifts: https://github.com/ionic-team/ionic-framework/blob/a0b3ef02af718e232246515bb873ad8c090fa55d/core/src/components/item-sliding/item-sliding.tsx#L309C51-L309C51
I believe the JS Pointer Events API has some logic built in that makes this code not necessary, though we'd need to update our gesture utility to use that API. We are tracking this in https://github.com/ionic-team/ionic-framework/issues/25595 (the issue references range, but it's the same underlying problem)
Does the performance issue happen on any iOS devices?
Yes, my iPad:
iPadOS: 16.5.1 Model: iPad Pro (11-inch) (2nd generation) Model #: MXDC2LL/A
Notice the stuttering / snapping:
https://github.com/ionic-team/ionic-framework/assets/362329/5521519a-abb9-4144-962e-edb08cf1f7d1
Again, not huge, but since we're getting into the weeds :)
Feature Request
Ionic version:
[x] 5.x
Describe the Feature Request
When rendering lists of items with complex structures including
ion-item-sliding
andion-item-options
, DOM rendering can be incredibly expensive. This is exacerbated on mobile devices with weaker G/CPUs and less memory where rendering performance is generally slow.It is not currently a "supported feature" to show the contents of items dynamically when sliding begins, then hide them again when sliding ends, to keep DOM node count and initial render under control.
Additionally, this makes it difficult to create custom components that use
ion-item-sliding
and insert slotted content intoion-item-options
elements, or even insert their own, conditionally viang-content
or a similar composition technique.There are a few problems that get in the way of this:
(ionDrag)
event, but it's a bit painful.options-start/end
is calculated at initial render time. If items are added/removed conditionally from these slots using*ngIf
or the like, the window for which the sliding item will open does not adjust its width. Therefore when the item is slid open, then elements are rendered (fade in animation to make it nice), the item will snap closed again because it does not believe there is anything to show.Describe Preferred Solution
ion-item-sliding
should:(ionDrag)
has been started. If new elements have been added, the width should be adjusted. AContentObserver
comes to mind so that a fixed-width pre-rendered placeholder is not required.Describe Alternatives
Basically hack this in and implement it manually, which is complex and difficult to get right due to the need to set a fixed width to a placeholder element inside the
ion-item-options
component so that once sliding starts, the item will know it has contents, set a width, and stay open/not snap closed again.Related Code
Additional Context