Timeline gc

wyatt777 commented

2019-10-22 00:30:59 +00:00

Member

Updated branch for #581

hj commented

2019-10-22 18:32:15 +00:00

Member

currently breaks if BE doesn't support the new API, would be great to detect if BE supports it or not, otherwise it completely breaks the FE if it's not supported.

feld commented

2019-10-23 22:38:26 +00:00

Owner

This would never be bundled with a BE that doesn't support it, so would it really matter? If someone chooses to run a newer develop FE than BE they should be aware of the risks.

hj commented

2019-10-24 06:59:23 +00:00

Member

currently there's no way to be aware of the risks since FE doesn't really have changelogs or versions like BE does and FE maintained by two-and-a-half people so there's not much versioning going on either.

I want at least some backwards compatibility to older BEs (or other BEs) - it might be buggy and unsupported but it shouldn't be completely broken for no good reason, and "trying to optimize some minor issue that's sometimes not even an issue, and we're not even sure if it's helpful or not" isn't a good reason.

currently there's no way to be aware of the risks since FE doesn't really have changelogs or versions like BE does and FE maintained by two-and-a-half people so there's not much versioning going on either. I want at least **some** backwards compatibility to older BEs (or other BEs) - it might be buggy and unsupported but it shouldn't be completely broken for no good reason, and "trying to optimize some minor issue that's sometimes not even an issue, and we're not even sure if it's helpful or not" isn't a good reason.

hj commented

2019-10-24 07:02:26 +00:00

Member

To put it differently - if this is merged as is - it would basically prevent me from working on FE for a while because I wouldn't be able to use Develop FE because my BE is outdated and needs update, however PleromaBE ~~develop~~ unstable has some bugs I would like to avoid, so I can't update BE right now, i can't update FE right now, so I can't really work on FE until it resolved all because of "optimization" that's not even clear is helpful or not right now.

To put it differently - if this is merged as is - it would basically prevent me from working on FE for a while because I wouldn't be able to use Develop FE because my BE is outdated and needs update, however PleromaBE ~~develop~~ unstable has some bugs I would like to avoid, so I can't update BE right now, i can't update FE right now, so I can't really work on FE until it resolved all because of "optimization" that's not even clear is helpful or not right now.

wyatt777 commented

2019-10-25 00:22:03 +00:00

Author

Member

I am confused about what I should be changing here. On initialize you want me to check if the new API url exists on BE, and either turn off or turn on GC?

It is not a very big thing to do, but I imagine this will occur a lot in the future.
Each new BE endpoint will need a check before FE can turn new features on.

I am confused about what I should be changing here. On initialize you want me to check if the new API url exists on BE, and either turn off or turn on GC? It is not a very big thing to do, but I imagine this will occur a lot in the future. Each new BE endpoint will need a check before FE can turn new features on.

wyatt777 commented

2019-10-25 04:05:58 +00:00

Author

Member

@hj @feld It now checks if BE supports it.

hj commented

2019-10-27 16:17:51 +00:00

Member

new instances might not even have statuses with id=1 since they will have flakes already, supposedly. Better use nodeinfo to check if it's supported or not. @kaniini was saying something about adding it.

hj commented

2019-10-27 16:22:33 +00:00

Member

otherwise instead of checking probably better to check actual error message/code.

which brings the question of "what will happen if one of the posts went MIA"

otherwise instead of checking probably better to check actual error message/code. which brings the question of "what will happen if one of the posts went MIA"

hj commented

2019-10-27 16:24:10 +00:00

Member

why delete here?

why `delete` here?

hj commented

2019-10-27 16:26:58 +00:00

Member

Array.isArray()?

`Array.isArray()`?

hj commented

2019-10-27 16:36:57 +00:00

Member

seriously? was timelineObject.visibleStatuses.length > max not enough? You might as well add

const checkIfOverMaxOrNot = (max) => {
  if (isOverMax(max)) {
    return true
  }
    return false 
  }
}

seriously? was `timelineObject.visibleStatuses.length > max` not enough? You might as well add ```js const checkIfOverMaxOrNot = (max) => { if (isOverMax(max)) { return true } return false } } ```

hj commented

2019-10-27 16:40:33 +00:00

Member

really not liking modifying vuex objects outside vuex modules

hj commented

2019-10-27 16:42:15 +00:00

Member

i don't see it being actually used anywhere, it's just set but never seem to be read

hj commented

2019-10-27 16:44:33 +00:00

Member

also not liking accessing DOM from vuex module

hj commented

2019-10-27 16:46:28 +00:00

Member

no longer using innovative isOverMax, huh?

no longer using innovative `isOverMax`, huh?

hj commented

2019-10-27 16:47:11 +00:00

Member

what does magic number 20 mean?

hj commented

2019-10-27 16:48:05 +00:00

Member

could've used vue to set style

hj commented

2019-10-27 16:48:54 +00:00

Member

maybe prefix it in some way?

hj commented

2019-10-27 16:51:58 +00:00

Member

what about posts below tho?

hj commented

2019-10-27 16:52:26 +00:00

Member

code is a massive clusterfuck, really. Sorry.

but then again existing timeline stuff is also somewhat clusterfuck as well...

code is a massive clusterfuck, really. Sorry. but then again existing timeline stuff is also somewhat clusterfuck as well...

wyatt777 commented

2019-10-28 00:09:38 +00:00

Author

Member

Sorry this was set for a different, date based approach. Will delete it.

wyatt777 commented

2019-10-28 00:10:44 +00:00

Author

Member

It is the arbitrary amount which is called for timeline pages.

wyatt777 commented

2019-10-28 00:18:50 +00:00

Author

Member

Thanks for the review! Time to clean the clusterfuck.

wyatt777 commented

2019-10-28 00:22:37 +00:00

Author

Member

very true

wyatt777 commented

2019-10-28 00:55:24 +00:00

Author

Member

statuses and statusObject are different object formats. The former is an array of objects with a field id. The latter is one big object with ids as keys. timelineObject.statusesObject this actually needs to be delete too. Making the change.

`statuses` and `statusObject` are different object formats. The former is an array of objects with a field id. The latter is one big object with ids as keys. `timelineObject.statusesObject` this actually needs to be delete too. Making the change.

wyatt777 commented

2019-10-28 01:03:06 +00:00

Author

Member

timelineData.futureIds.isArray is not a function

`timelineData.futureIds.isArray is not a function`

wyatt777 commented

2019-10-28 01:05:38 +00:00

Author

Member

This was useful for something before. Not needed now. So removing it.

wyatt777 commented

2019-10-28 01:08:33 +00:00

Author

Member

100 is also arbitrary. 20 means delete one page from the visible timeline.

wyatt777 commented

2019-10-28 01:13:36 +00:00

Author

Member

pinnedStatusIds? Different logic so I did not touch them.

`pinnedStatusIds`? Different logic so I did not touch them.

wyatt777 commented

2019-10-28 01:19:35 +00:00

Author

Member

pushed this one in small changes

wyatt777 commented

2019-10-28 01:19:58 +00:00

Author

Member

pushed this one in small changes

wyatt777 commented

2019-10-28 01:20:14 +00:00

Author

Member

removed in small changes

wyatt777 commented

2019-10-28 01:20:30 +00:00

Author

Member

removed in small changes

wyatt777 commented

2019-10-28 01:27:55 +00:00

Author

Member

If you have a suggestion for a different value let me know!

wyatt777 commented

2019-10-28 01:40:37 +00:00

Author

Member

Not a problem. Will set the change in a related module.

wyatt777 commented

2019-10-28 04:15:09 +00:00

Author

Member

For speed this was the more efficient approach.
To move it out, I need to set the Id in a state for the frontend to compute and watch.

Commiting the state in a for loop means we will need to await the commit to be complete, for each deleted ID (20 at a time).

I will move DOM changes into timeline.js for you to check. It is vanilla Vue, but it is slower :)

For speed this was the more efficient approach. To move it out, I need to set the Id in a `state` for the frontend to `compute` and `watch`. Commiting the state in a `for loop` means we will need to await the commit to be complete, for each deleted ID (20 at a time). I will move DOM changes into timeline.js for you to check. It is vanilla Vue, but it is slower :)

wyatt777 commented

2019-10-28 04:42:23 +00:00

Author

Member

pushed. moved into statuses module.

wyatt777 commented

2019-10-28 04:42:58 +00:00

Author

Member

pushed. as explained above into timelines.js

pushed. as explained above into `timelines.js`

wyatt777 commented

2019-10-28 04:43:24 +00:00

Author

Member

pushed. changed it make vue set the style :)

hj commented

2019-10-28 06:54:58 +00:00

Member

i mean you only seem to remove/reload posts above the viewport and reload then when gc block comes back into view, but not posts that below. Not entirely sure tho, I need to update my BE to properly test it, just looked weird.

hj commented

2019-10-28 06:55:17 +00:00

Member

Array.isArray(timelineData.futureIds)

`Array.isArray(timelineData.futureIds)`

hj commented

2019-10-28 07:04:27 +00:00

Member

i suggest using meaningfully named constants or probably even reusing existing stuff, i.e.

const PAGE_SIZE = 20

for (let i = 0; i < PAGE_SIZE; i++) {

import PAGE_SIZE from 'path/to/relevant/file'

i suggest using meaningfully named constants or probably even reusing existing stuff, i.e. ```js const PAGE_SIZE = 20 for (let i = 0; i < PAGE_SIZE; i++) { ``` ```js import PAGE_SIZE from 'path/to/relevant/file' ```

wyatt777 commented

2019-10-28 07:07:15 +00:00

Author

Member

{error: "Not implemented"} this is what we get currently if it is not in BE.
Will change it to fetch based on this. id=1 could be anything. It returns an empty array if the feature exists but cannot find the id.

`{error: "Not implemented"}` this is what we get currently if it is not in BE. Will change it to fetch based on this. id=1 could be anything. It returns an empty array if the feature exists but cannot find the id.

wyatt777 commented

2019-10-28 07:14:14 +00:00

Author

Member

Just pushed. I think this resolves it. Re-open if not.

wyatt777 commented

2019-10-29 01:45:13 +00:00

Author

Member

Thanks. This change will be in the next push.

wyatt777 commented

2019-10-29 01:48:36 +00:00

Author

Member

Changed in next push. Will keep this in mind for the future.

wyatt777 commented

2019-10-29 01:49:01 +00:00

Author

Member

Not sure what this one means. ??

wyatt777 commented

2019-10-29 01:52:37 +00:00

Author

Member

Below would load the normal way. Timeline maxId. Or they would be reloaded by id.

This is doing GC from the top.

Below would load the normal way. Timeline maxId. Or they would be reloaded by id. This is doing GC from the top.

hj commented

2019-10-29 07:07:50 +00:00

Member

now that I think about it it actually could be a bad idea.

originally i wanted it to be like id="status-${id}" instead of id="${status.id}"

but now that I think about it, you can easily end up with two statuses with same id and ids are supposed to be unique and break unexpectedly when there are duplicates.

now that I think about it it actually could be a bad idea. originally i wanted it to be like `id="status-${id}"` instead of `id="${status.id}"` but now that I think about it, you can easily end up with two statuses with same `id` and ids are supposed to be unique and break unexpectedly when there are duplicates.

wyatt777 commented

2019-10-30 23:44:04 +00:00

Author

Member

hmm, was not aware there can be duplicates for ids in the DB. What if I just make the id the index in the for loop?

hmm, was not aware there can be duplicates for ids in the `DB`. What if I just make the `id` the `index` in the for loop?

wyatt777 commented

2019-10-30 23:47:44 +00:00

Author

Member

Or :id="status.id + index"

Or `:id="status.id + index"`

wyatt777 commented

2019-10-31 00:27:30 +00:00

Author

Member

Actually, that will not work either. The way we call timelines in general depends on status ids being unique. Any suggestions for that??

wyatt777 commented

2019-10-31 00:38:55 +00:00

Author

Member

document.getElementById(this.lastVisible).scrollIntoView() this is the only possible place where an issue would occur (if there were two identical ids), with this code specifically. Added Try Catch here, if there is an error it will remain at the bottom of the reloaded posts.

`document.getElementById(this.lastVisible).scrollIntoView()` this is the only possible place where an issue would occur (if there were two identical ids), with this code specifically. Added Try Catch here, if there is an error it will remain at the bottom of the reloaded posts.

hj commented

2019-11-05 15:44:14 +00:00

Member

Little bit of testing after i've updated BE:

posts below aren't GC'd (which is the most performance-impactful thing i believe)
requests fetching for specific ids take up to 1 second to resolve which is very suboptimal and incredibly annoying
scroll locks up when scrolling up which is insanely annoying
shows empty space when scrolling up with PgUp/Home buttons (also try clicking navbar and iOS scroll to top shortcut (click iOS navbar/black bar with time and indicators))
see above - doesn't respect scrolling - try hitting Home button (or other means to scroll to top), you'll experience:
1. Top of page is shown with empty space in timeline
2. Scroll is locked (even if you navigate to another page like settings)
3. Nothing happens (is it frozen?)
4. Suddenly scroll jumps waaay to the bottom
the way statuses are gc'd makes no sense - just hit and hold End key for a while and then try to hit Home - posts are already GC'd even though they were viewed recently.
scrolling down also jumps scrolling a bit (might be a develop issue?)

Little bit of testing after i've updated BE: - posts below aren't GC'd (which is the most performance-impactful thing i believe) - requests fetching for specific ids take up to 1 second to resolve which is very suboptimal and incredibly annoying - scroll locks up when scrolling up which is insanely annoying - shows empty space when scrolling up with PgUp/Home buttons (also try clicking navbar and iOS scroll to top shortcut (click iOS navbar/black bar with time and indicators)) - see above - doesn't respect scrolling - try hitting Home button (or other means to scroll to top), you'll experience: 1. Top of page is shown with empty space in timeline 2. Scroll is locked (even if you navigate to another page like settings) 3. Nothing happens (is it frozen?) 4. Suddenly scroll jumps waaay to the bottom - the way statuses are gc'd makes no sense - just hit and hold End key for a while and then try to hit Home - posts are already GC'd even though they were viewed recently. - scrolling down also jumps scrolling a bit (might be a develop issue?)

hj commented

2019-11-05 15:50:37 +00:00

Member

So far it's far worse experience than what's already is there. It doesn't meet anything i specified in this comment #581

This is very barebones and sloppy implementation, UX-wise and code-wise. Garbage-collection needs a bit more serious approach.

So far it's far worse experience than what's already is there. It doesn't meet anything i specified in this comment https://git.pleroma.social/pleroma/pleroma-fe/issues/581#note_32640 This is very barebones and sloppy implementation, UX-wise and code-wise. Garbage-collection needs a bit more serious approach.

hj commented

2019-11-05 15:53:25 +00:00

Member

Additional tips:

there should be a debug option to show GC stats - how many posts were GC'd, how many statuses didn't make it back, how many statuses are in memory right now, how exactly are they visible, how long did GC and re-fetch took etc.
to avoid UI lockups GC should probably be done when page is idle, however detecting idleness might be a challenge.

Additional tips: - there should be a debug option to show GC stats - how many posts were GC'd, how many statuses didn't make it back, how many statuses are in memory right now, how exactly are they visible, how long did GC and re-fetch took etc. - to avoid UI lockups GC should probably be done when page is idle, however detecting idleness might be a challenge.

wyatt777 commented

2019-11-06 23:19:08 +00:00

Author

Member

Some of these are very subjective and emotional. I feel the passion!

insanely annoying

the way statuses are gc'd makes no sense

If you look at the history of the issue (related branches) you can see multiple different methods of resolving the issue: The app freezes if users scroll the page down too far. After multiple people checking including this most recent review by @hj , this is the most simple and straightforward way to do it.

Someone else can attempt again and have the same issues. This is because removing statuses with minimal damage to the timeline UI, has a few complications inherent to the problem.

If you remove or replace a status from a timeline with a blank shell, you need to save the space that it took up. If not it makes the UI erratic.
Timelines are a cluster fuck as @hj eloquently mentioned above. Even a simple solution will appear complicated to someone who is not directly working on the problem.

As for any perceived issue above, seem like trivial fixes when compared to what has been done so far :)

Some of these are very subjective and emotional. I feel the passion! > insanely annoying > the way statuses are gc'd makes no sense If you look at the history of the issue (related branches) you can see multiple different methods of resolving the issue: **The app freezes if users scroll the page down too far.** After multiple people checking including this most recent review by @hj , this is the most simple and straightforward way to do it. Someone else can attempt again and have the same issues. This is because removing statuses with minimal damage to the timeline UI, has a few complications inherent to the problem. 1. If you remove or replace a status from a timeline with a blank shell, you need to save the space that it took up. If not it makes the UI erratic. 2. Timelines are a cluster fuck as @hj eloquently mentioned above. Even a simple solution will appear complicated to someone who is not directly working on the problem. As for any perceived issue above, seem like trivial fixes when compared to what has been done so far :)

wyatt777 commented

2019-11-06 23:31:22 +00:00

Author

Member

#581 it is the specification for 5, 6, & 7.
It does 1, but needs to reload the statuses on scroll up.

#581 it is the specification for 5, 6, & 7. It does 1, but needs to reload the statuses on scroll up.

wyatt777 commented

2019-11-06 23:47:11 +00:00

Author

Member

I like these! But we are doing this the wrong way.

GC is meant to trigger to prevent the app from freezing, when a user scrolls down way too far through posts; It is an emergency cleanup. If we changed the threshold from 100, which is meant to be a proof of concept, to say 1000 statuses, no one would ever know this thing is silently working in the background.

Seems like a developer tool. Which certainly could be added in. But why?
UI lockup happens when users scroll back up to the threshold and is very momentary. This also occurs when users scroll down and posts are loaded. It is the very nature of loading statuses from a server.

I like these! But we are doing this the wrong way. GC is meant to trigger to prevent the app from freezing, when a user scrolls down way too far through posts; It is an emergency cleanup. If we changed the threshold from **100**, which is meant to be a proof of concept, to say **1000** statuses, no one would ever know this thing is silently working in the background. 1) Seems like a developer tool. Which certainly could be added in. But why? 2) UI lockup happens when users scroll back up to the threshold and is very momentary. This also occurs when users scroll down and posts are loaded. It is the very nature of loading statuses from a server.

hj commented

2019-11-06 23:56:07 +00:00

Member

The app freezes if users scroll the page down too far.

First things first, its only one third of the isse - the app also freezes if there are too many items in the timeline, which could be the case when frontend left running for too long time (try letting pleroma-fe run scrolled to top in background), and last part being that there's no limit to "known statuses", which is by itself a memory leak - pleromafe essentially stores ALL the statuses in memory, so if you leave it running for month it will still keep month old status in memory and it will never be removed, even though user will unlikely need to see it.

I mentioned it before - pefrorm gc when UI is idle or when amount of statuses to be cleared reaches insane numbers. Perform GC gradually and asynchronously with setTimeout() instead of batch-freeing everything. Implement proper stats tracker for it to see how well it performing, add an option to disable it completely for people who are annoyed by it too much.

Garbage collection doesn't need a "most simple and straightforward" solution, it needs a good and organized solution. This is also messy code-wise.

>The app freezes if users scroll the page down too far. First things first, its only one third of the isse - the app also freezes if there are too many items in the timeline, which could be the case when frontend left running for too long time (try letting pleroma-fe run scrolled to top in background), and last part being that there's no limit to "known statuses", which is by itself a memory leak - pleromafe essentially stores ALL the statuses in memory, so if you leave it running for month it will still keep month old status in memory and it will never be removed, even though user will unlikely need to see it. I mentioned it before - pefrorm gc when UI is idle or when amount of statuses to be cleared reaches insane numbers. Perform GC gradually and asynchronously with `setTimeout()` instead of batch-freeing everything. Implement proper stats tracker for it to see how well it performing, add an option to disable it completely for people who are annoyed by it too much. Garbage collection doesn't need a "most simple and straightforward" solution, it needs a good and organized solution. This is also messy code-wise.

wyatt777 commented

2019-11-06 23:58:39 +00:00

Author

Member

@hj I think you would have liked the other branch better then.
It did removal by lastViewed date. You could set a time and anything that was not viewed in that timeframe would start to be removed from the timeline.

#2129

@hj I think you would have liked the other branch better then. It did removal by lastViewed date. You could set a time and anything that was not viewed in that timeframe would start to be removed from the timeline. https://git.pleroma.social/pleroma/pleroma-fe/pulls/2129

hj commented

2019-11-07 00:02:07 +00:00

Member

You're basically swapping "UI freezes when scrolling down" with "UI freezes INTENTIONALLY and also badly when scrolling up", what good is that?

Because you're implementing garbage collection and you need to know how many statuses are in the memory right now and how many are about to be collected to see if GC is even working? It would also help you measure if 100 is better or 1000 as you mentioned above.

You're basically swapping "UI freezes when scrolling down" with "UI freezes **INTENTIONALLY** *and also badly* when scrolling up", what good is that? Because you're implementing garbage collection and you need to know how many statuses are in the memory right now and how many are about to be collected to see if GC is even working? It would also help you measure if 100 is better or 1000 as you mentioned above.

hj commented

2019-11-07 00:04:58 +00:00

Member

but you're already saving space for that spacer so what's the difference exactly?
as i said, it needs a good solution and a well organized one. The one where vuex store objects are modified via mutations/actions and not by editing objects directly, where vuex store doesn't access the DOM etc.

1. but you're already saving space for that spacer so what's the difference exactly? 2. as i said, it needs a good solution and a well organized one. The one where vuex store objects are modified via mutations/actions and not by editing objects directly, where vuex store doesn't access the DOM etc.

wyatt777 commented

2019-11-07 00:08:02 +00:00

Author

Member

So simply as a developer tool.

It will pause momentarily on scroll up as statuses are reloaded. This also currently happens on page scroll down. Things need to load from a server. Maybe have a similar rotating loading mark as page scroll down?

So simply as a developer tool. It will pause momentarily on scroll up as statuses are reloaded. This also currently happens on page scroll down. Things need to load from a server. Maybe have a similar rotating loading mark as page scroll down?

hj commented

2019-11-07 00:08:04 +00:00

Member

I feel like i would have liked working on the issue myself tbh. This is one of the "serious" issues that require serious approach and a lot of investigation and exploration. Reminds me of #565 in which I also had drastically different approaches to it, one of which was abandoned.

wyatt777 commented

2019-11-07 00:10:29 +00:00

Author

Member

I think that is very fair. If you ever do, you might be able to use a few ideas from this branch if you run into similar issues!

hj commented

2019-11-07 00:13:23 +00:00

Member

What good is that, again? Intentionally generating more serious bugs to work on later? Let's do a sloppy job fixing one issue and intentionally replace it with 3 more issues?

I can also say that when scrolling down you're just loading more posts into memory and vue gets slow when rendering 20x10 more elements in a tree with 100x10 elements, It is the very nature of Ｖｕｅ　ｒｅｎｄｅｒｉｎｇ

What good is that, again? Intentionally generating more serious bugs to work on later? Let's do a sloppy job fixing one issue and intentionally replace it with 3 more issues? I can also say that when scrolling down you're just loading more posts into memory and vue gets slow when rendering 20x10 more elements in a tree with 100x10 elements, *It is the very nature of* Ｖｕｅ　ｒｅｎｄｅｒｉｎｇ

wyatt777 commented

2019-11-07 00:20:24 +00:00

Author

Member

Loading statuses from a server will need to happen in any case, NO MATTER WHAT. We can make it load exactly how page scroll down is loaded; it will still need to reload and the page will still pause for a moment while it does.

Loading statuses from a server will need to happen in any case, **NO MATTER WHAT**. We can make it load exactly how page scroll down is loaded; it will still need to reload and the page will still pause for a moment while it does.

hj commented

2019-11-07 00:32:40 +00:00

Member

yes but you still can:

make loading less stiff - no scroll lock, no scroll jumping
take "scroll to top" case into consideration and not GC the topmost statuses
start loading missing statuses long before their placeholders enter view so that you don't see them when pressing pgup/pgdown or scrolling too quickly, at least not immediately
actually handle visiblity of statuses not just within timelines.

yes but you still can: 1. make loading less stiff - no scroll lock, no scroll jumping 2. take "scroll to top" case into consideration and not GC the topmost statuses 3. start loading missing statuses long before their placeholders enter view so that you don't see them when pressing pgup/pgdown or scrolling too quickly, at least not immediately 4. actually handle visiblity of statuses not just within timelines.

hj commented

2019-11-07 00:33:40 +00:00

Member

I mean, c'mon, seriously, do you even start localhost dev, try to use it, try to test it and say "yes, this is completely ok and very good, and also much better than it was before"?

wyatt777 commented

2019-11-07 00:44:06 +00:00

Author

Member

Scroll lock was made so that they do not go above statuses while they load. Could be a spinner.
Not a big issue. Make it work the same way as decided in 1.
If we change from 100 to 1000, this would be easier to do. Definitely do-able.
There are ideas in this branch that use intersection-observer to track visibility. I think this would be a reversion back to a previous state of this branch, or another WIP branch posted above. It had other issues.

"yes, this is completely ok and very good, and also much better than it was before"

This has been tried a few different ways. This was pushed for review as the best out of those.
I think if we actually spoke about the issue and the attempts at it, you would better understand the flow that got it here. There are legitimate reasons for these things.

1. Scroll lock was made so that they do not go above statuses while they load. Could be a spinner. 2. Not a big issue. Make it work the same way as decided in 1. 3. If we change from 100 to 1000, this would be easier to do. Definitely do-able. 4. There are ideas in this branch that use intersection-observer to track visibility. I think this would be a reversion back to a previous state of this branch, or another WIP branch posted above. It had other issues. > "yes, this is completely ok and very good, and also much better than it was before" This has been tried a few different ways. This was pushed for review as the best out of those. I think if we actually spoke about the issue and the attempts at it, you would better understand the flow that got it here. There are legitimate reasons for these things.

wyatt777 commented

2019-11-07 00:48:25 +00:00

Author

Member

@hj It is hard to filter through those threads now to see issues.

Consider other scroll cases. Like "scroll to top".
Start loading statuses before in view. Change from 100 to a larger number,
Add developer tool for GC.
Look into possible refactor "vuex store objects are modified via mutations/actions". I thought we already did this. Maybe there are a few left.

@hj It is hard to filter through those threads now to see issues. 1. Consider other scroll cases. Like "scroll to top". 2. Start loading statuses before in view. Change from 100 to a larger number, 3. Add developer tool for GC. 4. Look into possible refactor "vuex store objects are modified via mutations/actions". I thought we already did this. Maybe there are a few left.

wyatt777 commented

2019-11-07 00:54:48 +00:00

Author

Member

I mentioned it before - pefrorm gc when UI is idle or when amount of statuses to be cleared reaches insane numbers. Perform GC gradually and asynchronously with setTimeout() instead of batch-freeing everything.

If you remove a status anywhere else other than the top or bottom, you will run into issues.

Blank statuses while they reload back into view
Scroll bar jumping because a status was removed, even if you hold the space it will still jump a bit. A lot more with over 10 statuses removed.

> I mentioned it before - pefrorm gc when UI is idle or when amount of statuses to be cleared reaches insane numbers. Perform GC gradually and asynchronously with `setTimeout()` instead of batch-freeing everything. If you remove a status anywhere else other than the top or bottom, you will run into issues. 1. Blank statuses while they reload back into view 2. Scroll bar jumping because a status was removed, even if you hold the space it will still jump a bit. A lot more with over 10 statuses removed.

hj commented

2019-11-07 01:23:15 +00:00

Member

just... rethink the entire thing, the entire approach is just... wrong. I've outlined all the requirements in #581

Your approach is basically treating the symptom, not the root cause, i.e. doing the "UI freezes when too many statuses and loading down, let's make timeline have dummy spacer and freeze UI when scrolling up instead!" instead of "UI freezes when too many statuses, too many statuses is because we're leaking memory, we have too many statuses in memory but probably most of them aren't necessary, so let's free some memory by deleting statuses out of sight, and replace them with placeholders in UI".

just... rethink the entire thing, the entire approach is just... wrong. I've outlined all the requirements in https://git.pleroma.social/pleroma/pleroma-fe/issues/581#note_32640 Your approach is basically treating the symptom, not the root cause, i.e. doing the "UI freezes when too many statuses and loading down, let's make timeline have dummy spacer and freeze UI when scrolling up instead!" instead of "UI freezes when too many statuses, too many statuses is because we're leaking memory, we have too many statuses in memory but probably most of them aren't necessary, so let's free some memory by deleting statuses out of sight, and replace them with placeholders in UI".

kaniini commented

2019-11-08 01:21:18 +00:00

Member

This needs a rebase, but will be going in afterward. I am making an executive decision.

hj commented

2019-11-08 01:43:43 +00:00

Member

i'm vetoing it unless it has an option to disable the GC which should be OFF by default.

kaniini commented

2019-11-08 02:58:04 +00:00

Member

I agree with the option to disable the GC. We should also handle capability negotiation. Can we get the GC disable option? I'll add a feature to nodeinfo (multifetch) for capability negotiation.

I agree with the option to disable the GC. We should also handle capability negotiation. Can we get the GC disable option? I'll add a feature to nodeinfo (`multifetch`) for capability negotiation.

kaniini commented

2019-11-08 18:53:39 +00:00

Member

multifetch feature has been added to nodeinfo in BE. Please add code to disable this feature if the multifetch feature is not present. Please also add an override setting to allow an admin to disable the GC if necessary. It will be merged after these constraints are added and validated.

`multifetch` feature has been added to nodeinfo in BE. Please add code to disable this feature if the `multifetch` feature is not present. Please also add an override setting to allow an admin to disable the GC if necessary. It will be merged after these constraints are added and validated.

hj commented

2019-11-08 19:10:34 +00:00

Member

i'd say not just admins but users too, i.e. typical instance-default option.

settings code has been improved, should be easier to implement options now.

i'd say not just admins but users too, i.e. typical instance-default option. settings code has been improved, should be easier to implement options now.

wyatt777 commented

2019-11-09 01:55:43 +00:00

Author

Member

Ok. I will make those changes and post back here for review.

wyatt777 commented

2019-11-12 03:14:35 +00:00

Author

Member

multifetch and instance-default added. @hj @kaniini

settings code has been improved, should be easier to implement options now.

I accidentally added it with both before and after rebase.
LocalizedValue portion was confusing in settings.js, took some time to guess the value needed to be undefined in modules/config.js. Needs more comments.

`multifetch` and instance-default added. @hj @kaniini > settings code has been improved, should be easier to implement options now. I accidentally added it with both before and after rebase. `LocalizedValue` portion was confusing in `settings.js`, took some time to guess the value needed to be `undefined` in `modules/config.js`. Needs more comments.

hj commented

2019-11-12 07:48:48 +00:00

Member

use yarn instead of npm, remove package-lock.json

use `yarn` instead of `npm`, remove `package-lock.json`

hj commented

2019-11-12 07:58:19 +00:00

Member

    timelineGC ({ rootState, commit, rootGetters }, { timelineObject }) {
      const multiFetch = rootState.instance.multiFetch
      const gcEnabled = rootGetters.mergedConfig.garbageCollector

you should use merged config which accomodates for instance defaults

```suggestion:-2+0 timelineGC ({ rootState, commit, rootGetters }, { timelineObject }) { const multiFetch = rootState.instance.multiFetch const gcEnabled = rootGetters.mergedConfig.garbageCollector ``` you should use merged config which accomodates for instance defaults

hj commented

2019-11-12 08:09:11 +00:00

Member

I'll look more into it in the evening (hopefully)

shpuld commented

2019-11-12 12:50:34 +00:00

Member

Ok, I went through this and got an explanation of what it actually does. Weighing in with my opinions on this as I promised.

I think this is not good, a lot of complexity for something that causes more usability issues than it fixes. The good part is simplifying timeline dom which gets more and more complicated and more and more expensive to update the longer you scroll down, the bad parts are:

it's not real GC for the statuses store,
having to refetch and wait for information just to scroll up, really bad for mobile data,
code quality just isn't there.

A better approach in my opinion would be to do virtual scrolling and GC separately. Virtual scrolling without deleting any statuses from stores would just make sure vue has to do less dom rendering and manipulation. I'm pretty sure that's the real problem behind any bad performance the more you scroll down. Then if you don't actually get rid of the statuses in the memory, you don't have to refetch them when you scroll back up.

This sort of an approach could be attempted in a more simple manner by having statuses (or conversations) simplify themselves into a div instead of a complicated status when out of view keeping dom much simpler (but possibly still have some perf issues if the amount of divs is stupid high, but that's a big doubt from me).

For real GC to keep memory usage lower (vs. DOM performance that everything above is), the correct approach would be to check the vuex statuses store periodically, check if statuses are being used on any timelines/notifications (might as well consider clearing timelines that are not visible to make this simpler and reduce memory use even further) and remove them. Might need something like reference counting to make it performant, but possibly not necessary.

For this MR, I'd honestly abandon it and approach the virtual scrolling and GC completely separately. Also any kind of performance optimization needs some benchmarking. You want to know that you're optimizing the right parts before doing anything complex.

Ok, I went through this and got an explanation of what it actually does. Weighing in with my opinions on this as I promised. I think this is not good, a lot of complexity for something that causes more usability issues than it fixes. The good part is simplifying timeline dom which gets more and more complicated and more and more expensive to update the longer you scroll down, the bad parts are: 1) it's not real GC for the statuses store, 2) having to refetch and wait for information just to scroll up, really bad for mobile data, 3) code quality just isn't there. A better approach in my opinion would be to do virtual scrolling and GC separately. Virtual scrolling without deleting any statuses from stores would just make sure vue has to do less dom rendering and manipulation. I'm pretty sure that's the real problem behind any bad performance the more you scroll down. Then if you don't actually get rid of the statuses in the memory, you don't have to refetch them when you scroll back up. This sort of an approach could be attempted in a more simple manner by having statuses (or conversations) simplify themselves into a div instead of a complicated status when out of view keeping dom much simpler (but possibly still have some perf issues if the amount of divs is stupid high, but that's a big doubt from me). For real GC to keep memory usage lower (vs. DOM performance that everything above is), the correct approach would be to check the vuex statuses store periodically, check if statuses are being used on any timelines/notifications (might as well consider clearing timelines that are not visible to make this simpler and reduce memory use even further) and remove them. Might need something like reference counting to make it performant, but possibly not necessary. For this MR, I'd honestly abandon it and approach the virtual scrolling and GC completely separately. Also any kind of performance optimization needs some benchmarking. You want to know that you're optimizing the right parts before doing anything complex.

wyatt777 commented

2019-11-12 13:41:20 +00:00

Author

Member

Thanks Shpuld for the review!

The points you bring up are great.
I think @hj said he may want to try a fix for this.
It would be great to see the differences in approach when that is out.

I expect, other attempts at the this problem will also be mired in the same complications that I faced. Removing statuses / timelines in vuex gets messy quickly for UI. Changing things in the dom to divs or anything else also gets messy quickly.

I will review in the morning and probably close the MR then.

Thanks Shpuld for the review! The points you bring up are great. I think @hj said he may want to try a fix for this. It would be great to see the differences in approach when that is out. I expect, other attempts at the this problem will also be mired in the same complications that I faced. Removing statuses / timelines in vuex gets messy quickly for UI. Changing things in the dom to divs or anything else also gets messy quickly. I will review in the morning and probably close the MR then.

wyatt777 commented

2019-11-15 02:49:58 +00:00

Author

Member

I reviewed this again and did the updates.

Virtual scrolling without deleting any statuses from stores would just make sure vue has to do less dom rendering and manipulation.

We would need benchmarking to verify this.

having to refetch and wait for information just to scroll up, really bad for mobile data,

If we do need to remove/add statuses, this will be the case for any method we choose.

code quality just isn't there.

This has been reviewed by four people. With a lot of revisions.

I think with the restrictions in place to choose turning it on, the review that has been done, and the current need for a solution, it should be pushed.

If someone makes a full solution in the future we can revert / alter it.

I reviewed this again and did the updates. > Virtual scrolling without deleting any statuses from stores would just make sure vue has to do less dom rendering and manipulation. We would need benchmarking to verify this. > having to refetch and wait for information just to scroll up, really bad for mobile data, If we do need to remove/add statuses, this will be the case for any method we choose. > code quality just isn't there. This has been reviewed by four people. With a lot of revisions. I think with the restrictions in place to choose turning it on, the review that has been done, and the current **need** for a solution, it should be pushed. If someone makes a full solution in the future we can revert / alter it.

shpuld commented

2019-11-15 07:36:35 +00:00

Member

We would need benchmarking to verify this.

Yes, and that makes me wonder why one would even begin to build an even more complex and compromiseing solution without benchmarking to find the actual cause of freezing first. I'm sorry but this is insane.

If we do need to remove/add statuses, this will be the case for any method we choose.

Not if you only remove statuses that are not on timelines, then the behavior doesn't get worse than what we have already. There's a world of difference when you load statuses again by just using regular timeline loading vs. doing a hacky multi-fetch of individual statuses.

This has been reviewed by four people. With a lot of revisions.

I didn't leave comments in the code itself because I think it would be a waste of my time and your time when the approach itself isn't valid. It's a shame so much time and effort has been spent on this approach.

There's definitely something wrong if you say there's a need for solution but it's also the kind of a solution we don't want to force on people because it also makes things worse. A proper fix for a problem would be the kind that we could safely enable for all. I really don't want to add this complexity into the code base if I don't think it's worth it. The way it's programmed is not simple or easily maintained, seems quite bug prone with the way things are being deleted and the amount of nested if checks.

When actually trying it out on my laptop, the UI still gets unresponsive when the gc kicks in after 500 statuses and when it starts deleting stuff, the timeline flashes once that pause is over, so it's not really smooth. This combined with completely breaking scrolling up (seriously you can't click on the top bar to go on top anymore, it freezes and takes you back down) makes it just not an improvement in any way. I don't mean to disrespect your work but when I said it'd be better to start over with a new approach I really mean it.

> We would need benchmarking to verify this. Yes, and that makes me wonder why one would even begin to build an even more complex and compromiseing solution without benchmarking to find the actual cause of freezing first. I'm sorry but this is insane. > If we do need to remove/add statuses, this will be the case for any method we choose. Not if you only remove statuses that are not on timelines, then the behavior doesn't get worse than what we have already. There's a world of difference when you load statuses again by just using regular timeline loading vs. doing a hacky multi-fetch of individual statuses. > This has been reviewed by four people. With a lot of revisions. I didn't leave comments in the code itself because I think it would be a waste of my time and your time when the approach itself isn't valid. It's a shame so much time and effort has been spent on this approach. There's definitely something wrong if you say there's a *need* for solution but it's also the kind of a solution we don't want to force on people because it also makes things worse. A proper fix for a problem would be the kind that we could safely enable for all. I really don't want to add this complexity into the code base if I don't think it's worth it. The way it's programmed is not simple or easily maintained, seems quite bug prone with the way things are being deleted and the amount of nested if checks. When actually trying it out on my laptop, the UI still gets unresponsive when the gc kicks in after 500 statuses and when it starts deleting stuff, the timeline flashes once that pause is over, so it's not really smooth. This combined with completely breaking scrolling up (seriously you can't click on the top bar to go on top anymore, it freezes and takes you back down) makes it just not an improvement in any way. I don't mean to disrespect your work but when I said it'd be better to start over with a new approach I really mean it.

shpuld commented

2019-11-15 09:49:39 +00:00

Member

I did some actual testing with the scrolling performance when you scroll down super deep and created a new issue about it: #711

In short, memory isn't the problem, messing with a huge list of status components is.

I did some actual testing with the scrolling performance when you scroll down super deep and created a new issue about it: https://git.pleroma.social/pleroma/pleroma-fe/issues/711 In short, memory isn't the problem, messing with a huge list of status components is.

shpuld commented

2019-11-15 09:52:01 +00:00

Member

Closing this now, this is trying to fix the wrong things to attempt to solve the problem of scrolling becoming really laggy and unstable when scrolling through thousands of statuses.

See my comments in this MR and the new issue I created about it: #711

Closing this now, this is trying to fix the wrong things to attempt to solve the problem of scrolling becoming really laggy and unstable when scrolling through thousands of statuses. See my comments in this MR and the new issue I created about it: #711

Pull request closed

Please reopen this pull request to perform a merge.

Rows
Columns

Timeline gc #2244

Pull request closed