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

How to insert() / update() many items #282

Closed
GKersten opened this issue Aug 12, 2021 · 23 comments
Closed

How to insert() / update() many items #282

GKersten opened this issue Aug 12, 2021 · 23 comments
Labels

Comments

@GKersten
Copy link

Hey! First of all, great package! Last couple of weeks I have been investigating which Virtual Scroller to use within our application, and so far we ended up using ngx-ui-scroll.

The current feature we are building is a notification center, in which we have multiple groups/categories of notifications.
Each group can have many notifications (1 - 1000+). We solved the grouping, using the flattening strategy that @dhilt proposed to use in other examples of Grouping inside a scroller.

We want to be able to first see all groups in a collapsed manner: just showing 1 notification per group and than have a button to "show all".
This show all, can result in inserting 100s or 1000s of items. As we have a virtual scroller we were thinking this should be doable.

But what we are seeing is that whenever we use the update() or insert() api of the adapter, it actually tries to render all items in the DOM, than afterwards we have the option to clip() and remove the items, but it takes quite some time to do this whole thing.

Is there a more efficient way to handle this?

I've create a very simplified repro here:
https://stackblitz.com/edit/ngx-ui-scroll-insert-many-repro

There are 2 scenarios that I am having issues with:

  1. Click the "Insert 1000" button, notice that the performance is not so good, since it is rendering all items to the dom.
  2. After inserting the first 1000 items, scroll all the way to the bottom, Insert an other 1000 items (default at index 4, so outside the current visible buffer), now scroll through the list and the items appear to be messed up. I am not totally sure if I broke my own logic for syncing with the datasource, but when you click "reload" you see that the items appear in correct order.

Reload seems to give the performance we are looking for, but the downside is that we want to let the user stay in the current scroll position + also don't have the "flash" that is happening when you reload all items.

@GKersten GKersten changed the title How to insert() / update() many items with solid performance How to insert() / update() many items Aug 12, 2021
@GKersten
Copy link
Author

I just updated my stackblitz example. Introduced some new logic in which I try to optimize the insert.

I tried to optimize the insert() by first checking the buffered items, than limit the amount of items to insert based on the current amount of buffered items (or by using the bufferInfo.defaultSize as fallback).
This seems to work good at first, but running into multiple issues using this strategy:

  • I still need to perform a reload(), because If we don't reload, the scroller will not update the maxIndex, so we won't load any new data when scrolling down or up. Can we just update the maxIndex, to prevent the reload? I want to prevent a reload() since it will "flash" the whole view, and rerenders all items

  • I needed a work-around for reload at index: I am also seeing some weird behaviour with the reload() method. Somehow it seems to always be 10 index too low, so I needed to created a work-around to always add 10 to the desired index in order to reload at the correct position?

  • Still trying to further investigate; but mainly the first optimized insert seems to work correct, but after some more, it seems like the datasource is getting out of sync with my actual data. Not sure if it is in my logic or not. Will do further testing to get a clear scenario.

@dhilt
Copy link
Owner

dhilt commented Aug 13, 2021

@GKersten I'm going to dig into this during weekend, but two brief things before:

  • you may change maxIndex at runtime with experimental Adapter.fix({ maxIndex}) API, demo;
  • synchronization of the external Components data and the internal Scroller's data is the greatest challenge in this story, indexes must concat, increase and flow very carefully during the Datasource updates (like Adapter.insert) and this is a developer responsibility and a source of big number of issues; your case is very interesting and I'll be ready to assist, especially you have provided stackblitz!

@GKersten
Copy link
Author

Thanks! I will continue to improve it, and indeed this is a great challenge to keep in sync, but so far for us is the only way to solve it, so trying to find the most efficient way of handling this.

Will keep making changes to the stackblitz to build up the scenario, as it is also for me a better way to separate the logic of this feature from our actual application.

Will first check out the adapter.fix(), I already used it in our application to update individual items that did update their data, but should not be recreated during the .update()

@GKersten
Copy link
Author

Update: I used the Adapter.fix({ maxIndex}) and that seems to solve the main issue I was having.
Of course my main question would remain if this logic I applied here is making sense, would love to get a review on that, or get feedback if there are some things that can be improved.

Current example in the Stackblitz seem to work pretty well, will still need to test some edge cases.

@dhilt
Copy link
Owner

dhilt commented Aug 18, 2021

@GKersten I looked into the demo, here's my fork: https://stackblitz.com/edit/ngx-ui-scroll-insert-many-repro-forked. Changes and concerns:

  1. Datasource insertion (insertToDataSource) is fixed, the last item was lost per each insert.
  2. Limiting of the inserted items is improved. This logic is extracted into a separate method (getItemsToInsertLimit), which implements an approach used in specs. It is more accurate and precise.
  3. Relates to p.2. You misinterpreted the meaning of bufferInfo.defaultSize, and I see that this is missed in the readme-doc. The defaultSize is the height of 1 item, which is dynamic and which is used by the Scroller to predict how many items should be fetched via Datasource.get. This demo reveals the meaning of this variable, but it uses "default" word; it was done before exposing this value via Adapter.
  4. The core of the Insert method is updated. It uses Adapter.update now, as it allows to encapsulate the in-buffer logic and get rid of extra conditions. This refactoring shed light on the virtualization problem, which is the last point.
  5. Virtualization problem. We have 10 items initially and want to inject 100 new items. We insert 30 new items after item 5 and get [1, 2, ..., 5, 11, 12, ..., 40, 6, 7, ..., 10] array. Then we clean up the tail. Let's say we get [1, 2, ..., 5, 11, 12, ..., 22]. Cleaned items [23, 24, ..., 40, 6, 7, ..., 10] are virtualized. But not-inserted 70 items [41, 42, ..., 110] should be virtualized too. Currently we just expand maxIndex value, but this way we lost the order! Scroller thinks that the virtual items are [23, 24, ..., 40, 6, 7, ..., 10, 41, 42, ..., 110], while the Datasource has [23, 24, ..., 110, 6, 7, ..., 10]. When you do scroll, the engine minimizes and eliminates the error, it adapts to new circumstances and the over-scrolled result looks good. But I believe the error will be significant if we deal with different heights items. Virtualization must count indexes. Adapter.fix({ maxIndex }) is inappropriate solution...

There are 3 methods in the Adapter API, which provide stable virtualization for some cases: append (by predicate and eof), prepend (by predicate and bof) and remove (by indexes). It is possible to implement a workaround for p.5: insert 30 items, don't clip but remove the tail manually by Adapter.remove({ items: [6, 7, ..., 10] }) and append the explicit sequence of items virtually via Adapter.append({ eof: true, items: [41, 42, ..., 110, 6, 7, ..., 10] }). Before appending the maxIndex should be expanded to reset the internal eof flag, otherwise the list will be rendered immediately.

I didn't try this workaround, you may. What I want is to think what can be done here in general, maybe extend the Adapter insert/update methods to allow virtualization, maybe something else.

@GKersten
Copy link
Author

Hey @dhilt, I am trying to put something together with your proposed work arounds with append and prepend.
Something I currently see:

When I already scrolled all the way to the bottom, and you do an append, even with eof: true it will add the items directly to the DOM. Is this correct?

Also, you mention:

which provide stable virtualization for some cases: append (by predicate and eof), prepend (by predicate and bof)
What do you mean by predicate in these cases? I just see an option for providing an array of items to add?

@dhilt
Copy link
Owner

dhilt commented Aug 19, 2021

@GKersten You are right twice: 1) if EOF is reached, items will be appended and rendered immediately; 2)Adapter.append wants items array + eof flag. So, if you want to append some items virtually when you are at EOF, you need to do something like that:

  async doAppendVirtually(items: Data[]) {
    this.appendItemsToDatasource(items); // if this was not done before
    await this.datasource.adapter.relax();
    if (this.datasource.adapter.eof) { // drop EOF
      const maxIndex = this.datasource.adapter.bufferInfo.absMaxIndex;
      this.datasource.adapter.fix({ maxIndex: maxIndex + items.length });
    }
    await this.datasource.adapter.append({
      items, eof: true
    });
  }

@GKersten
Copy link
Author

@dhilt one more question: I am struggling a bit with the .remove().
I am trying to implement what you suggest, instead of .clip() we remove the tail so we keep the virtualization correct.

Will we need to remove all the indexes after the inserted items? Or only the once that are visible in the current buffer?
If for example I have 2000 items in my list, I insert 10 items at index 10, will I need to remove all individual indexes after index 20 (index 10 + 10 inserted), to keep the virtualization correct? This will mean I need to get an array of about 1980 indexes that I send to the .remove(). Since the predecate only works on the currently visible items in the buffer. Does this make sense?

I mainly notice issues with the virtualization when the item height is set to a high value (bigger than the viewport height for example) and I perform these inserts.

@dhilt
Copy link
Owner

dhilt commented Aug 23, 2021

@GKersten If you insert only 10 items, I believe you don't need to remove anything. If you insert, say, 1000 items and want to split this update into real 10 set and virtual 990 set, then the algorithm might be as follows:

  • insert 10 set into Datasource
  • relax
  • insert 10 set into Scroller via Adapter.insert
  • relax
  • there are "tail" items after inserted: some of them are rendered ("tail1") and some are virtual ("tail2")
  • remove the tail from Datasource
  • remove the tail from Scroller via Adapter.remove({ items }); this can be hard because of "items" list signature, when you need to pass indexes as an array; maybe it is a good idea to add "startedFrom" option to pass only 1 index...
  • relax
  • concatenate 990 set and removed tail (get new 990+ set)
  • append 990+ set to Datasource
  • increase Scroller's max index to satisfy 990+'s last item via Adapter.fix({ maxIndex })

Datasource updates are separate here, perhaps this is not necessary, but it should provide the most stable approach without additional investigating and testing. Also, I gave up using the Adapter.append API. Setting the max index value should be enough. Alternative (optimized) algorithm:

  • insert 10 set and remove "tail1" set in Datasource
  • relax
  • insert 10 set and remove "tail1" set in Scroller via Adapter.update
  • relax
  • remove "tail2" set from Datasource
  • remove "tail2" set from Scroller via Adapter.remove({ items }); "startedFrom" looks interesting...
  • concatenate 990 set and removed tail (990+)
  • append 990+ set to Datasource
  • increase Scroller's max index to satisfy 990+'s last item via Adapter.fix({ maxIndex })

I don't like fix-maxIndex... There might be some virtual updating calls like Adapter.append({ amount }) or even Adapter.insert({ startedFrom, amount }) that may drastically reduce the procedure.

Please post an updated demo, if you see problems, I'll take a look!

@GKersten
Copy link
Author

Again thanks for the help @dhilt

I have been busy trying to implement what you suggest, also did a fork again on Stackblitz to have something simple to talk about and improve. In general it seems like this approach is working, I need to do more extensive testing with different heights, and I will need to implement multiple manipulations on the datasource (I want to listen to an observable which can add/remove/update items with each emit on the observable.)

For now to keep it a bit simpler, I tried to implement what you suggest above, and I came to this demo, which implements both of your options. I think I like the .update() best for my goals.

You can check my updated StackBlitz over here:
https://stackblitz.com/edit/ngx-ui-scroll-insert-many-with-remove

I have some remarks / questions / issues, could you take a look at them?

Main issue with both Option 1 and 2:

  • Inserting anything within or after the visible buffer seems to work fine, but when I insert something before the visible buffer, the scroller remains white until I scroll. I think this makes sense since all items are removed and added again. But would expect the scroller to update itself if it is not presenting items, but there is actually padding left. Is there a way to trigger the page load for the current viewport + padding? I tried doing things like clip() or check() but that didn't trigger what I needed.

General remark:

  • Simplified logic for calculating the insert limit, see implementation of getItemsToInsertLimit()

Potential improvement on Option 2:

  • not sure if needed to split the removal of tail1 and tail2 from datasource in 2 distinct steps, could we also remove all tail items from datasource directly?

@dhilt
Copy link
Owner

dhilt commented Aug 26, 2021

@GKersten Basically your demo looks ok, the only 1 important point I see: lastVisibleIndex is wrong concept, because "tail1" means rendered items, i.e. buffered, so I replaced it with adapter.bufferInfo.lastIndex. Other changes are minor. My updates are here: https://stackblitz.com/edit/ngx-ui-scroll-insert-many-with-remove-forked.

I didn't fix virtual backward insertion case (when you meet the empty space problem), the more I look at it, the more I want to implement virtual insertion at the Adapter API level. There are too many points, for example, you'll have to adjust scroll position after expanding the backward padding element. Of course, this should not be a developer responsibility, the lib should handle this. The workaround for this case might be as follows:

  • make sure that the insertion will be virtual out of the topmost border of the Buffer
  • make all necessary changes in the Datasource
  • reset the Scroller via Adapter.reset({ settings }), where settings should contain actual params: expanded maxIndex and new startIndex reflecting current Adapter.firstVisible.$index item

Another option would be to decrease minIndex implicitly: Datasource gets negative indexes, its items shifts into negative area, Scroller drops its internal minIndex to -Infinity and starts getting backward items from scratch. This is different behaviour + handling negative indexes might be not intuitive (look this demo for natural index-based operations in negative area).

@GKersten
Copy link
Author

GKersten commented Aug 26, 2021

@dhilt I noticed that when we did virtual backward insertion, the datasource.adapter.firstVisible.$index becomes undefined, I used this to determine:

make sure that the insertion will be virtual out of the topmost border of the Buffer
After this check I am now doing the .reset(), does it make sense?

Also noticed the shortcut return; that you introduced when we are not slicing the new inserted items into visible and virtual once (when we don't exceed the limit). I added a clip() & fix({ maxindex }) like below, to improve and fix some issues:

      await adapter.relax();
      await adapter.clip(); // to prevent unlimited items added to the dom
      await adapter.fix({ maxIndex: this.data.length }); // Fix for: insert items outside current buffer will not increase maxIndex

I update the StackBlitz to test above changes with your improvements applied: https://stackblitz.com/edit/ngx-ui-scroll-insert-many-with-remove

I am now still running into 1 more issue: The index of buffered items are not updated when insert is done outside the buffer. To repro:

  • Scroll to bottom
  • insert 1 item at index 0
  • insert 1 item at index 99
  • scroll down again, notice "# 100" is visible twice.

@GKersten
Copy link
Author

Also, I agree that all these work-around do need seem like the desired way of handling all this with this package. So would be great if we can solve it internally at Adapter API level.

Could you give me any insights on how this would go? I would love to help but not sure where to start.

We still think that this library is best choice for our needs, but would like to find the right approach: stick with the work arounds vs help with/wait for internal implementation of this virtual inserts.

@dhilt
Copy link
Owner

dhilt commented Aug 27, 2021

@GKersten

  1. Adapter.firstVisible provides the first visible item, not the first rendered. Please look at this Viewport doc, the diagram has the following areas: visible rendered (dark-blue), hidden rendered (light-blue), virtual (white). Need to use Adapter.bufferInfo.firstIndex.

  2. await Adapter.relax() is not necessary if you do await Adapter.foo() right before; relaxing is incapsulated in the foo-promise (except Adapter.fix, which is synchronous).

  3. When you insert 1 item at 0 position being at the bottom, you make a virtual insert, which is buggy. Scroller's indexes must increase, but they don't. I updated my fork, it has "reset" workaround, which works as I can see:

    if (afterIndex < adapter.bufferInfo.firstIndex) {
      this.insertToDataSource(afterIndex, count);
      await adapter.reset({
        settings: {
          minIndex: 0,
          maxIndex: this.data.length,
          startIndex: adapter.firstVisible.$index + count
        }
      });
      return;
    }

@dhilt
Copy link
Owner

dhilt commented Aug 27, 2021

@GKersten Regarding implementation of the virtual insert functionality at the Adapter layer, let me think a bit, I need to draw up a plan... And I really appreciate you for your suggestion to participate in this story, let's get back on the next week, it should become clearer.

@dhilt
Copy link
Owner

dhilt commented Sep 1, 2021

@GKersten Hello! I've just released ngx-ui-scroll v2.2.1 and ready to switch to this issue. We need to discuss a new API proposal. The Adapter.insert method should receive a few more options to provide virtual insertion, my guess it may be beforeIndex and afterIndex:

Adapter.insert({
  items: MyItem[],
  before?: ItemsPredicate,
  after?: ItemsPredicate,
  beforeIndex?: number,
  afterIndex?: number,
  decrease?: boolean
})

Only one of before/after/beforeIndex/afterIndex options is allowed. Predicates (before/after) work with Buffer. Indexes (beforeIndex/afterIndex) do virtual things. If the value of beforeIndex/afterIndex belongs to Buffer, the insertion will be real, not virtual. For example, if we have [20..50] items in the Buffer, the following two calls will be equivalent:

Adapter.insert({ items, before: ({ $index }) => $index === 35 });
Adapter.insert({ items, beforeIndex: 35 });

So if you want to insert many items, you'll still need to split the flow and perform 2 separate actions: in-buffer and virtual.

async insertBefore(index, items) 
  const { adapter } = this.datasource;
  let virtualList = items, inBufferList = [];
  await adapter.relax();
  if (index >= adapter.bufferInfo.firstIndex && index <= adapter.bufferInfo.lastIndex) {
    const { virtual, inBuffer } = this.splitItemsToInsert(list);
    virtualList = virtual;
    inBufferList = inBuffer;
  }
  if (inBufferList.length) {
    this.insertBeforeDS(index, inBufferList);
    await adapter.insert({ items: inBufferList, indexBefore: index });
    await adapter.clip({ forwardOnly: true });
    index += inBufferList.length;
  }
  this.insertBeforeDS(index, virtualList);
  await adapter.insert({ items: virtualList, indexBefore: index });
}

This approach is not super simple, but seems more intuitive and compact than what we tried earlier. No additional removals or appending are needed. What do you think? Regarding implementation, a great challenge might be to extend the Adapter.update API before, because Adapter.insert uses it under the hood. Adapter.update works with Buffer only, it has no virtualization, and right now I have no idea how this should be done.

@GKersten
Copy link
Author

@dhilt sorry for the delay, just back from holiday :)

But to reply to your proposal, I agree that this will make it more intuitive, I think still I would have expect that the library would take more care about calculating what items should be inserted directly in the buffer, and which items should be inserted virtual, after all this whole library is meant to help you solve virtualization of scrolling elements?

But for me it's hard to say if this really makes sense, I can imagine it's a bit more high level than what the current library is solving for you, and could seen like a wrapper on top of the current API's.

For me in anyway these changes would help in building our solution in a more readable / maintainable way.

Did you get any more insights on this or ETA on these changes?

@dhilt
Copy link
Owner

dhilt commented Sep 22, 2021

@GKersten The work is started: dhilt/vscroll#27. I'm improving the related API in parallel (remove and append), so it could take some time. I guess it may take 1-2 weeks to see the result.

@GKersten
Copy link
Author

@dhilt Great news! Will keep an eye out, please let me know if there is anything I can help with

@dhilt dhilt mentioned this issue Sep 24, 2021
5 tasks
@dhilt
Copy link
Owner

dhilt commented Oct 1, 2021

@GKersten ngx-ui-scroll v2.3.0 has been published, Adapter.insert API received two new options allowing insertion by index:

interface AdapterInsertOptions<Data = unknown> {
  items: Data[];
  before?: ItemsPredicate<Data>;
  after?: ItemsPredicate<Data>;
  beforeIndex?: number; // new!
  afterIndex?: number; // new!
  decrease?: boolean;
}

If the index is out of the Scroller's buffer, items will not be injected into DOM, they will be virtualized. The readme-doc and the insert-demo contain details. Please try v2.3.0, there was a lot of development and refactoring!

@dhilt
Copy link
Owner

dhilt commented Oct 30, 2021

@GKersten I have implemented the insert-many-and-limiting demo based on the new Adapter.insert API: https://stackblitz.com/edit/ngx-ui-scroll-insert-many-and-limiting. The flow becomes more clean and simple.

private async insertToScroller(items, index, count) {
  const { adapter } = this.datasource;
  let limit = this.getItemsToInsertLimit();
  limit = count < limit ? count : limit;
  const isVirtual = index < adapter.bufferInfo.firstIndex || index > adapter.bufferInfo.lastIndex;

  // immediate Scroller insertion: fully virtual or not limited
  if (isVirtual || count <= limit) {
    return await adapter.insert({ afterIndex: index, items });
  }

  const limitedItems = items.slice(0, limit);
  const restItems = items.slice(limit);

  // insert limited set into Scroller
  await adapter.insert({ afterIndex: index, items: limitedItems });

  // cut buffered items after the inserted ones
  await adapter.clip();

  // virtualize the rest set
  await adapter.insert({ afterIndex: index + limitedItems.length, items: restItems });
}

The potential concern is that "clip" must cut enough items for the index + limitedItems.length value should be greater than the value of the adapter.bufferInfo.lastIndex property after "clip". Otherwise, the last "insert" will be performed in-buffer, not virtually. And this is the getItemsToInsertLimit method responsibility to provide proper limiting count value. If we can't guarantee it, then "clip" + "virtual insert" approach should be updated/replaced with something more complicated.

@stale
Copy link

stale bot commented Jan 31, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed within seven days if no further activity occurs. If it needs to remain open, add the "permanent" label.

@stale stale bot added the stale label Jan 31, 2022
@stale
Copy link

stale bot commented Feb 27, 2022

This issue has been automatically closed because it was stale.

@stale stale bot closed this as completed Feb 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants