-
-
Notifications
You must be signed in to change notification settings - Fork 457
EXPERIMENT: Dramatically improve table scrolling performance #4860
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
base: develop
Are you sure you want to change the base?
Conversation
seancolsen
left a comment
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.
Wow, this is really interesting and cool @zackkrida! I'm very impressed!
What this PR shows me is that we have been doing more re-rendering than I thought. What a fantastic opportunity for optimization! The main point that I didn't understand before was this:
From Svelte’s perspective, that looked like a full reorder
I was assuming that the already rendered rows were not re-rending on scroll. This PR seems to prove my assumption wrong. I'll admit I still don't have as firm a grasp on why Svelte seems to be behaving differently that I was expecting. So I'm curious to potentially dig a little deeper and poke at it.
Your solution of keying on the array index is a clever way to avoid re-mounting a bunch of components. But I'm a little wary of this approach. What it seems to be doing when scrolling is preserving the entire tree of mounted components for each row but changing the props passed to those components. I can see how this is faster than re-mounting the components. But it feels a bit strange to me intuitively. It's almost as if we're "scrolling the data" while keeping the sheet fixed. But my intuition is telling me that it could be even faster if we work out a way to preserve the components in place (without changing their props) and just translate their rendered position in the sheet. This might mean re-designing the VirtualList component. I'm not sure. But if the row components remain mounted and end up receiving new props on scroll then I'm worried we're going to be chasing strange bugs (like the focus bug that you already identified).
As an example, here's another one:
- Enter edit mode in a cell.
- Scroll the sheet.
- Expect that the cell remains in edit mode and scrolls with the sheet. (This was the behavior before this PR.)
- Instead, observe that edit mode is opened on different rows as you scroll. This is because edit mode is a state that is local to the cell.
My inclination here would be apply our learnings from this experiment to redesigning a virtual list system. Our current VirtualList system is designed to be quite flexible. It's completely decoupled from the Sheet system. I'm a little curious if we might need something that's a bit more tailored to our specific needs in this sheet case. Or perhaps the virtual list system needs to be a bit more tailored to Svelte itself. If I recall, @pavish had originally built the virtual list system based off of some library designed for React.
I'm very curious to see what @pavish thinks about all this. He's much more familiar with the virtual list system.
While we're on this topic, I do also want to mention that if I were to design our sheet system from scratch I would use a virtual list exclusively without using pagination. This would be similar to "infinite scroll", but not "infinite". The scroll bar would visually express to the user the position of their subset of data within the entire table/query. If you scroll to the bottom, you'd get to the end of the table. Not that I think we ought to make that change now (because it would be a lot of work). But I mention it as food-for-thought if we end up re-thinking some of our virtual list system.
|
When I first implemented the VirtualList component (four years ago or earlier), I researched a bunch of them and tried different approaches.
Your approach in this PR was the second approach from the above list. When I tested these approaches, we had little to no interactivity and the most simplest row component which only rendered the data:
Then I realized that inorder to use the second approach, we had to move all logic to JS instead of using the dom and what the framework provides, including keyboard events, component states etc., This was the issue both you and @seancolsen were mentioning in the description/comment. At the moment, I saw no reason to go with the second approach and chose the first one. I also noticed that every popular library was using the first approach and I ported react-window. We are now at the point where the first approach is terrible for performance because we have a ton of components that we create and destroy for every row and cell. The Row components are too heavy now for the first approach. I'm glad and happy to see from your PR that the second approach works quite well no matter the complexity of the components. The way forward is to go through the codebase and move all dom specific logic to the TS layer (similar to @seancolsen's work with the context menu). Once we do that and ensure that we don't use any dom specific work or internal component state based logic within the Row and Cell components, we can use this approach. I don't see us merging this PR in now, however, this gives us enough motivation to start working towards it right away. Response to @seancolsen:
This was something we considered at the very beginning and the reason we went away from that was because we needed a realiable way to fetch records from the backend without duplicates between each request (or reconcile them in the frontend). It also lead to discussions on whether to use a cursor/keyset pagination or the limit-offset pagination for fetching records, and we chose the simplest one to implement. |
|
While trying to see if I had documented this somewhere (sadly, it doesn't look like I have), I saw the very first PR that implements virtual scroll: #363, and @zackkrida you've reviewed that PR! That was a pleasant surprise, I had no memory of it. That PR also had infinite scrolling as @seancolsen mentioned where the rows were fetched based on scroll position, which seems to have lead to empty rows while scrolling when the data was not yet received. I didn't remember that I did that first before moving to the limit-offset pagination. I remember testing performance using the |
7acd4ef to
817856e
Compare
817856e to
6eb9449
Compare
|
@seancolsen thanks for giving this a look! I wanted to address some of your thoughts specifically.
This is actually the same (and only) bug I've identified so far. I misused the term "focus" many times yesterday 😅 — I was referring to edit mode. I've also fixed it in 311c400 by moving the id of the cell in edit mode outside of the CellWrapper and into the SheetContext.
Yes! "Scrolling the data" is precisely what most production virtualization libraries that work with JS frameworks do! React Window, TanStack Virtual, etc. It’s by far the most efficient approach, because it’s the only way to avoid constant component mount/destroy while keeping the DOM small and stable.
If we did that, we'd still have to render and maintain every single row component in the DOM at once. That's expensive, especially if we implement infinite scrolling. The browser still has to allocate memory, compute layout, and maintain compositing layers for every element, regardless of visibility. So even though visually we'd just be "moving" things, the DOM would still be huge, and scrolling would eventually bottleneck on layout and paint work instead of component updates.
Very fair point! This is the inherent tradeoff of virtualization. The fix is to lift any state that should persist across interactions (like edit mode) out of the row components and into shared sheet-level state. I think that's a better long-term architecture anyway, since the cell's "edit" state is really a property of the sheet UI, not the component instance itself. |
|
@pavish thanks for the history here (which it turns out I was a part of). @seancolsen and @pavish, as I mentioned in my comment to Sean, I pushed a fix to the edit mode issue (311c400). I've tested this extensively and I do not see any bugs of any kind. I'd really like to fix table scroll performance and I do think this is the right approach to take. It will allow us to reimplement infinite scrolling, if desired. My personal preference would be to merge this after additional, heavy testing, and get it in the next release. Alternatively: We could look into disabling virtual scrolling entirely. This would, perhaps excluding cases with a page size of 500 and very wide tables, improve Mathesar's performance considerably over the current live approach. We would replace constant re-renders with a larger DOM. This can also be tested with a very simple diff that renders all rows instead of creating the virtual windows of partial rows: diff --git a/mathesar_ui/src/components/sheet/virtual-list/listUtils.ts b/mathesar_ui/src/components/sheet/virtual-list/listUtils.ts
index 7db1c05c9..02397770b 100644
--- a/mathesar_ui/src/components/sheet/virtual-list/listUtils.ts
+++ b/mathesar_ui/src/components/sheet/virtual-list/listUtils.ts
@@ -223,7 +223,8 @@ function getItemStyle(props: Props, index: number): Item['style'] {
function getItemsInfo(props: Props): ItemInfo {
const { itemKey, itemCount, isScrolling } = props;
- const [startIndex, stopIndex] = getRangeToRender(props);
+ const [startIndex, stopIndex] = [0, itemCount - 1];
+
const items: Item[] = [];
if (itemCount > 0) {
for (let index = startIndex; index <= stopIndex; index += 1) { |
|
@zackkrida That is awesome! I also went through the entire list of cell related components to check how much state we're maintaining within components that are not "external data" backed. I'm glad that we don't really have a lot of such component states. There are some places where we're maintaining dom elements in props. I don't see them affecting us, but there might be weird edge cases we're not facing yet. The good thing is that we can easily move them inside functions they're used. I still need to test the PR extensively, and think of possible edge cases related to event handlers. However, I'm confident we can get this PR merged for this release!
This would delay the initial rendering of the table, and there would be delays for every additional request (refresh, pagination, filtering etc.,). You could test it out and notice that the browser freezes for a second or so. Our row components are not lean enough to render 500 of them at the same time. I would not recommend doing this.
I don't think this is the case. All these libraries provide the data that is visible, but do not modify the DOM themselves. They most certainly don't re-use the same DOM elements, that is definitely not the most efficient approach for simple lists. They are efficient for us because our list has too much component creation/destruction logic.
|
You're of course right, @pavish. Oops! What I meant to write (I was hopping between messages and lost track of my thought) was that some of the commercial "data grid" plugins I've used in the past (think something like ag grid) use the DOM node reuse strategy, and that it would also be compatible with those popular virtualization libs I mentioned. I'll leave my original message as-is and hope @seancolsen reads on. As for everything else you wrote--sounds great! |
|
I'm still feeling a little wary of an approach which reuses the same DOM elements with different data. I just have a hunch that we'll end up with weird bugs. We would need to be certain that we don't have any local state in any row/cell-level components. That's not an assumption that we've taken thus far. So we'd need to do a fair bit of auditing to ensure that we can bring the codebase into conformance with that assumption. And going forward, I don't see any easy way to enforce that assumption, other that developer-facing documentation and diligent code review. So I worry something could fall through the cracks. @zackkrida I see that you've added some changes to this PR in pursuit of addressing the "edit mode" local state. And I can see that the behavior is different now. But the behavior is still weird to me. (I've not looked at the diff — just the user-facing behavior.) If I enter edit mode then make a change then the scroll the sheet, I expect the cell editor to remain open with my change. But what I see here is that the cell editor closes without saving my changes. (I don't like that because I could lose my work just by scrolling.) Further, I'm noticing that when I scroll up, the active cell end up being shifted down 5 rows consistently. So I lose my place in the sheet. Strange. I even noticed a time when the content of the cell I was editing changed to the content of a different cell in the same column. It was really weird. I was not able to produce it again though. I can think of another piece of local state: custom dropdowns. As an example, we have these for boolean cells with custom labels. This doesn't play nicely with this new scrolling approach. It's similar to the "edit mode" state. We have this for custom dropdown for non-image files. If we use the row seeker in the sheet, we'd have it there too. We've also considered implementing something similar for hyperlinks to follow the way other apps deal with clicking hyperlinks. Strangely, the date picker does not seem to pose a problem in this PR based on my current testing (but it is abysmally slow to scroll the sheet when the date picker is open, so I do wonder what's happening). We could potentially disallow scrolling under these circumstances when the user has "something open" at the cell level. But I can't think of a good way to implement that logic declaratively, or "across the board". It seems to me that we'd need to imperatively handle that on more of an ad-hoc basis, which I don't like. Plus, it would hinder usability. In addition to any component-level state we might also need to refactor DOM-state out of our approach — and really out of our mindset here. For example, we currently we rely on the active cell being focused at the DOM level in order to propagate a number of different events. This way, pressing Enter moves the focused cell into edit mode. Pressing Menu (a rather esoteric keyboard key — but one that I love) opens the context menu on the focused cell. As it stands in this PR, keeps the active cell on the correct data row but not the focused cell. We could fix this by re-focusing the active cell on scroll. Or we could change our approach to event handling altogether. We could keep the DOM element for the sheet focused and handle events there. That would help perf too because we wouldn't be adding and removing so many event handlers. |
|
I have been thinking quite a lot about this change, and I still think it's worth the effort to get this done. I've been still wrapping my head around potential bugs and maintainability concerns where we have to be mindful of not letting state & events into any Row & Cell related components, which is going to be hard to do if we want these components to be composable. Despite my earlier excitement of trying to get this into the current release, I'd like some more time to assess if we can make the row & cell components display only (a view layer without any state & events), and the cell active state (edit mode) to be decoupled from the cell itself and placed at the Sheet layer. By this, I don't mean just the data, I mean the entire DOM component of the active cell. I think we should have a call on this, and aim to get this done and fully tested for the next release. |
|
@pavish I like how you're thinking about this. And @seancolsen I agree with many of your concerns. I think we should slow down a bit and aiming for doing something with table performance in 0.8.0 rather than rushing anything into 0.7.0. I'm out next week, but I'd love to discuss this on a call when I'm back. Or open to this friday if that works for the two of you... |
This is a small performance experiment which should dramatically improve the performance of scrolling through large tables in Mathesar. This change appears simple but it does have some implications. First, I'll explain the change and what it does, then I'll explain the caveats.
Screencast.From.2025-10-09.19-48-42.mp4
Screencast.From.2025-10-09.19-52-16.mp4
How this works
Previously, each visible row in our virtual table was keyed by its data identity (
item.key). This meant that every time the visible window/page changed, the set of keys changed as well. From Svelte’s perspective, that looked like a full reorder, so each scroll required destroying and recreating row components. Every scroll event triggered a full Svelte flush, repeated mount/destroy work, and frequent garbage collection.With this change, rows are now keyed by their array index (
(i)), not by data identity. This maintains a fixed pool of row components and simply updates their props as the active rows change.As a result, Svelte no longer performs expensive teardown and rebuild cycles on every scroll, leading to dramatically smoother performance when navigating large tables.
What this breaks
As far as I can tell, this only has one behavior implication we'd need to figure out. Namely, if you have a cell focused and are editing its value, and then scroll, the cell focus will "jump" around to other cells.
The way Mathesar normally behaves in this scenario is that the active cell gets unfocused when the current virtual window is changed.
I think this issue could be fixed with minimal effort, I just need to think about how 😅
Testing notes
I notice the difference is most dramatic in Firefox, which has typically-worse rendering performance than Chrome. Still, even in Chrome the difference is noticeable.
Next steps
@seancolsen and @pavish, I'd love you to look at this and think about anything else it might break. Hopefully I haven't missed anything obvious.