-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Virtualization support #24179
Virtualization support #24179
Conversation
5a9fe7a
to
9d668c5
Compare
This is a really superb start, @MackinnonBuck! There's a lot about this I like very much. There may be quite a lot of CR feedback here, which is because it's a super-important part of the Blazor 5.0 product and so I'll be quite keen on the details :) Hope that's OK. |
/// <summary> | ||
/// Describes services enabling platform-specific virtualization. | ||
/// </summary> | ||
public interface IVirtualizationService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I recognize some benefits of the IVirtualizationService
design concept, I'm not yet 100% sold on it :) It seems to be based on the following:
- All rendering platforms can be assumed to provide an implementation for it
- There are use cases for sharing the exact same
<Virtualization>
component type across multiple rendering platforms
I'm not sure either of those are necessarily true. At least, I haven't yet thought of a case where someone would be building a component that's high-level enough to have UI opinions like "I should be virtualized" while also low-level enough to be applicable across multiple rendering platforms.
The second concern I have is that the abstraction hasn't exactly worked out without leaking. The VirtualizeBase
code embeds some web-specific concepts (i.e., the notion of a div
and a CSS style
attribute). If we were keen to avoid this abstraction leakage, would that potentially work out most easily by keeping VirtualizeBase
in M.A.Components
and then implementing a Virtualize
in M.A.Components.Web
that can directly do JS interop without having to go through an IVirtualizationService
?
For the abstraction to be as general as possible, it's worth considering whether the whole concept of "spacer" elements could be limited to the .Web
implementation. Other UI platforms might have a native implementation of virtualization that can work more conveniently without needing any spacer things, and in fact might not even be able to have any equivalent to a spacer depending on their rules about allowed UI widget hierarchies.
If the end result is that there is very little non-web-specific stuff left, then arguably we might have taken a misstep by thinking there could be a non-platform-specific version virtualization (which would be my mistake in the original suggestion). But TBH I don't know, maybe there is enough to make the platform-independent base abstraction remain worthwhile!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would be nice to remove IVirtualizationService
if possible. I've gone ahead and committed a different approach that combines Virtualize
and InfiniteScroll
, which removes most of the need for a service to provide shared functionality across components. I did end up leaving in VirtualizeBase
, as I do believe there are still a few concepts that can be generalized across platforms.
src/Components/Components/src/Virtualization/IVirtualizationHelper.cs
Outdated
Show resolved
Hide resolved
/// Provides functionality for rendering a virtualized, infinite list of items. | ||
/// </summary> | ||
/// <typeparam name="TItem">The <c>context</c> type for the items being rendered.</typeparam> | ||
public sealed class InfiniteScroll<TItem> : VirtualizeBase<TItem> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely open to distinguishing the <Virtualize>
component from <InfiniteScroll>
. You've pointed out some advantages in terms of having fewer possible invalid combinations of parameters.
It would be great to fully understand the tradeoffs we'd hit if we considered an alternative approach where infinite scrolling was purely a usage pattern within a single <Virtualize>
component. Example:
- If you specify an
ItemHeight
(orItemSize
) then we automatically do random-access seeking to arbitrary points in the set as the user scrolls, and render a spacer-after that expands to whatever height is needed to make the scrollbar represent what we know about the full but finite scroll range. Presumably each time the developer fetches some more data, they could choose to update anItemCount
on the<Virtualize>
, which would update the effective scroll range. - Or if you don't specify
ItemHeight
, then the spacer-after is only as high as it naturally is according to a<SpacerAfter>
template parameter provided by the developer, so in effect there's no random access because there's no way to scroll further than the<SpacerAfter>
element. It's up to the developer to fetch as many additional items as they want each time that becomes visible, and optionally signal that they've run out of items (maybe by updatingItemCount
to match the total already fetched) at which point we'd no longer render the spacer-after.
This is not to say I'm fixated on this alternative approach. The approach you already have here might be better, but I want to be explicit about the tradeoffs :) The functionality we ship in M.A.Components
needs to remain supported as-is forever so we feel uncertain about parts of it, I'd err on the side of simply skipping those pieces (e.g., infinite scrolling) until we get more confident in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done my best to implement what you've suggested here, albeit with some differences - most notably the omission of ItemCount
. I tried the idea of using ItemCount
to hint to Virtualize
whether there are more items to fetch, but from a usability standpoint, I personally found it to be a bit unintuitive (often the ItemCount
provided was just the number of items fetched, plus 1, and removing that plus 1 when there were no items remaining complicated the code). I also found that it was difficult to make scrolling behave correctly when ItemCount
was larger than item count of the internal collection. Perhaps there is a nice solution to this - I didn't put in too much time investigating this specific problem. If the idea was to allow users to scroll freely without getting stuck waiting for items to load, I think this should be feasible by generating placeholder items using ItemsProvider
, and replacing them after the asynchronous "fetching" operation completes. Please let me know what your thoughts are on this, since there might be some use cases I haven't yet considered.
|
||
protected override IEnumerable<RenderFragment> GetItems(int start, int count) | ||
{ | ||
if (start + count >= _loadedItems.Count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a risk that we ask the developer to fetch some more, and they do fetch some more, but it's still not enough to fill the vertical height of the component? If so what happens then? I'm wondering if we'll get stuck because the spacer will remain visible and hence no subsequent event will fire to say it's becoming visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may have been a bug with my previous implementation, but now this scenario behaves properly. The MutationObserver
is looking at the "after" spacer, so even if the number of initial items fetched isn't enough to fill the visible space, the MutationObserver
will catch that the spacer changed height and reset the IntersectionObserver
, and since the bottom spacer is still intersecting with the container, OnBottomSpacerVisible
will be invoked, which initiates another data fetch.
And, if the total number of items isn't enough to fill the visible space, Virtualize
won't get stuck in a loop of constantly requesting more items because the "after" spacer will stop changing height when no more items are loaded.
However, there's currently no good way to proactively fetch more items and alert Virtualize
that it has more items to render (at least when using ItemsProvider
). That's probably something worth looking into.
@SteveSandersonMS, thanks for the feedback! I've made some changes based on your suggestions. There are still some items I need to address, and I need to stress-test this some more, but let me know if you think this is a step in the right direction. I'm happy to consider reintroducing some elements from my previous commits that got removed with this new approach if you think it's desirable. Thanks! |
|
||
using System.Threading.Tasks; | ||
|
||
namespace Microsoft.AspNetCore.Components.Web |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although we definitely want Virtualize
itself in this namespace, I'd consider it a more open question whether we want related types like ItemsProviderDelegate
in that same namespace, or whether there should be a .Virtualization
namespace for those. That's because the name ItemsProviderDelegate
is pretty generic and it's not obvious that it's specific to virtualization. Alternatively we could consider renaming it to VirtualizationItemsProvider
or similar.
Let's not delay this PR on that question though. We can think about it during API review and do a small follow-up update if necessary.
private ValueTask<ItemsProviderResult<TItem>> DefaultItemsProvider(ItemsProviderRequest request) | ||
{ | ||
return ValueTask.FromResult(new ItemsProviderResult<TItem>( | ||
Items!.Skip(request.StartIndex).Take(request.Count), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to figure out whether Enumerable.Skip
has an optimization for indexable types where it goes directly to the requested index, or whether it actually walks the enumerator N times. In these sources it seems to walk the enumerator, and this discussion implies that's what it does too. However I set up a little microbenchmark and was unable to observe any extra perf cost from walking a long collection, even with a billion elements.
If it turns out that walking an ICollection
's enumerator to implement Skip
is an O(N) operation that becomes problematic for large N on WebAssembly, we could consider a future enhancement where we implement our own CollectionEnumerator that knows how to skip in O(1).
Maybe you or someone else reading this knows better about the internals of Skip
and what it ends up costing. I'm not suggesting we need to optimize this right now!
src/Components/test/testassets/TestServer/PrerenderedStartup.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superb! Great to see you were able to resolve the scrolling weirdness. This looks very clean and powerful now.
I know you might be planning to add more test cases, but I'm approving now to make sure you're not held up.
…mabuc/virtualization
…mabuc/virtualization
Summary of the changes
Usage
Virtualizing an in-memory list:
Virtualizing an asynchronous item source:
If someone wanted to cache asynchronously-fetched items in memory so
<Virtualize>
wouldn't fetch the same item twice, they could cache items after they're fetched in theItemsProvider
, returning any cached items synchronously.Potential future features
ItemsSize
unspecifiedAddresses #23092