message_list: Don't always cache "All messages" view. #29740
Conversation
web/src/message_list.js
Outdated
| @@ -72,6 +69,10 @@ export class MessageList { | |||
| // the user. Possibly this can be unified in some nice way. | |||
| this.reading_prevented = false; | |||
|
|
|||
| // Used to determine whether to preserve the rendered state | |||
| // when this message list is not the current message list. | |||
| this.preserve_rendered_state = user_settings.web_home_view === "all_messages" && !this.narrowed; | |||
There was a problem hiding this comment.
Probably worth extending the comment to mention that this doesn't get live-updated when the setting changes, but that's probably OK, as it's just an optimization?
There was a problem hiding this comment.
Oh! I forgot to add a comment here. I was thinking if user had all_messages as the default view in the current session, then it doesn't make sense for us to no longer have it's rendered state be present if they changed the default view. Since, user likely still uses the combined feed view and the faster rendering of combined feed view could be a better experience if we have already done the work of keeping it updated.
There was a problem hiding this comment.
I can update preserve_rendered_state in event handlers code if you think otherwise.
There was a problem hiding this comment.
Yeah I think the current logic is good, we should just make sure the subtlety is documented.
There was a problem hiding this comment.
Tweaked the comment a bit:
diff --git a/web/src/message_list.js b/web/src/message_list.js
index f8eac445dc..2f5fd55b8e 100644
--- a/web/src/message_list.js
+++ b/web/src/message_list.js
@@ -68,11 +68,14 @@ export class MessageList {
// the user. Possibly this can be unified in some nice way.
this.reading_prevented = false;
- // Used to determine whether to preserve the rendered state
- // when this message list is not the current message list.
- // This is intentionally not live updated right now since user
- // likely is a frequent user of the combined feed view if they
- // had combined feed set as their default view in the current session.
+ // Whether this message list's is preserved in the DOM even
+ // when viewing other views -- a valuable optimization for
+ // fast toggling between the combined feed and other views,
+ // which we enable only when that is the user's home view.
+ //
+ // This is intentionally not live-updated when web_home_view
+ // changes, since it's easier to reason about if this
+ // optimization is active or not for an entire session.
this.preserve_rendered_state =
user_settings.web_home_view === "all_messages" && !this.narrowed;
| @@ -112,7 +104,7 @@ export function update_recipient_bar_background_color(): void { | |||
| } | |||
|
|
|||
| export function save_pre_narrow_offset_for_reload(): void { | |||
| if (current === undefined) { | |||
| if (!current?.preserve_rendered_state) { | |||
| return; | |||
| } | |||
There was a problem hiding this comment.
This check isn't obviously equivalent...
I guess the old logic would return if the current view was inbox/recent, or if was "all messages".
The logic returns if the current view is inbox/recent (current == undefined), or if preserve_rendered_state is false, i.e. not "all messages"? I suspect this is misconverted.
But it does look correct for the caller to no longer try to check narrow_state.has_shown_message_list_view, since it appears that this will noop in the event that no such view has been shown. (I guess that wasn't true in a previous code base state -- we'd have current = home when loading the app to inbox before current === undefined was made a possible state, right?). Could potentially do the narrow_state.has_shown_message_list_view removal in a prep commit, but this feels readable enough as written.
There was a problem hiding this comment.
Added this comment there:
// Only save the pre_narrow_offset if the message list will be cached if user
// switches to a different narrow, otherwise the pre_narrow_offset would just be lost when
// user switches to a different narrow. In case of a reload, offset for the current
// message is captured and restored by `reload` library.
There was a problem hiding this comment.
I'm going to leave further cleanup on this for a follow-up; we should definitely try to clean it up to make sense, but it's probably a very subtle detail, so I don't think it's a blocker for merging.
| // Used to determine whether to preserve the rendered state | ||
| // when this message list is not the current message list. | ||
| this.preserve_rendered_state = user_settings.web_home_view === "all_messages" && !this.narrowed; | ||
|
|
||
| return this; | ||
| } | ||
|
|
There was a problem hiding this comment.
So there's one block of logic that I'm not whether it needs attention:
update_muting_and_rerender() {
// For the home message list, we need to re-initialize
// _all_items for stream muting/topic unmuting from
// all_messages_data, since otherwise unmuting a previously
// muted stream won't work.
//
// "in-home" filter doesn't included muted stream messages, so we
// need to repopulate the message list with all messages to include
// the previous messages in muted streams so that update_items_for_muting works.
if (this.data.filter.is_in_home()) {
this.data.clear();
this.data.add_messages(all_messages_data.all_messages());
}
I'm not sure that hack still works correctly with the message_lists.home logic no longer being deeply tied to all_messages_data in the message_fetch logic -- it seems like that could very easily result in your not having the currently selected message in view, if that's not present in all_messages_data.
It's a messy problem -- we had this performance-favorable model previously that we maintain the full set of messages in all_messages_data, and thus always have the ability the rework the data if muting state changes -- but now I think if you scroll up in the view, we'll ask the server for data that we don't end up storing in all_messages_data. Hrm.
There was a problem hiding this comment.
Some version of this problem in theory applies to any search view with -is:muted among the filters; but those other views are going to be rare search views where people don't totally expect live-update. (And there's plenty of precedent: The has:image search view doesn't live update when someone edits a message to now have an image, and that seems totally OK).
I think there's basically two possible solutions:
- In an unmuting or message-moving event causes some messages to (possibly) now match the combined feed view, and it's cached, we ask the server for full copies of any messages we didn't have copies of locally, a la
maybe_add_narrowed_messages, as a deferred live-update step. - Another theory, which I'm not at all sure is a good idea, would be that we arrange that the cacheable
filter.is_in_homeview actually uses the realall_messages_data as its.message_list_data`? Since they're both singletons, I think it'd be workable ... but seems messy.
But it would require some preparatory refactoring; this function at least accesses .items, not .all_items on the all_messages_data object:
function load_local_messages(msg_data) {
// This little helper loads messages into our narrow message
// data and returns true unless it's visibly empty. We use this for
// cases when our local cache (all_messages_data) has at least
// one message the user will expect to see in the new narrow.
const in_msgs = all_messages_data.all_messages();
msg_data.add_messages(in_msgs);
return !msg_data.visibly_empty();
}
There was a problem hiding this comment.
We do fetch the muted data for the combined feed view as we have always done. See load_messages. Modified this comment:
// Otherwise, we don't pass narrow for the combined feed view; this is
// required to display messages if their muted status changes without a new
// network request, and so we need the server to send us message history from muted
// streams and topics even though the combined feed view's in:home
// operators will filter those.
There was a problem hiding this comment.
Also, removed this:
// For the home message list, we need to re-initialize
// _all_items for stream muting/topic unmuting from
// all_messages_data, since otherwise unmuting a previously
// muted stream won't work.
//
// "in-home" filter doesn't included muted stream messages, so we
// need to repopulate the message list with all messages to include
// the previous messages in muted streams so that update_items_for_muting works.
if (this.data.filter.is_in_home()) {
this.data.clear();
this.data.add_messages(all_messages_data.all_messages());
}
Since could actually lead to a bug where the combined feed view has discontinued messages since all messages data is anchored to latest message and the combined feed view is anchored to the first unread message.
There was a problem hiding this comment.
Yeah, so to be concrete, I think the thing that's worse/regressed is that if you have no unreads and go to the "All messages" view, and just scroll up, you'll do all these fetches from the server that in theory could extend the set of cached messages in all_messages_data (as we do with the "load more" button in the Recent Conversations view), but we will no longer do so.
I think that's probably OK to take no action on for this PR. Opened #29841 to track this.
|
This looks like a great direction! I posted a few dozen comments, including identifying a few small prep PRs that could shrink this a bit. But I think #29740 (comment) is the main complex issue that might require significant reworking that I've noted in reading this. |
|
This reduces rendering of all messages view from 7s to 3s when it is the default view on 6x CPU slowdown. |
|
Looks like we will have to rewrite the tests for the |
| // If adding some new messages to the message tables caused | ||
| // our current narrow to no longer be empty, hide the empty | ||
| // feed placeholder text. | ||
| narrow_banner.hide_empty_narrow_message(); | ||
| } | ||
|
|
||
| if (this.narrowed && !this.visibly_empty() && this.selected_id() === -1) { | ||
| if (!this.visibly_empty() && this.selected_id() === -1) { | ||
| // The message list was previously empty, but now isn't | ||
| // due to adding these messages, and we need to select a | ||
| // message. Regardless of whether the messages are new or |
There was a problem hiding this comment.
I think these checks might still need a conditional on whether the view is currently rendered instead of this.narrowed; if it isn't, then selecting a message or hiding the banner will be applied based on the properties of an invisible background view, and that seems buggy? So maybe we need this === message_lists.current, effectively?
There was a problem hiding this comment.
It might make sense to just have an explicit this.is_current_message_list boolean on this object that we can check here; it'd likely be easy for narrow.ts logic to maintain that, and would avoid needing a messy import of some sort of access message_lists.current.
There was a problem hiding this comment.
Pushed a commit for this in #29871.
Important changes in this commit: * We only cache message list for "Combined feed" if it is the default view. * We modify existing handling of home message list code so that it can be used to for any message list that we want to cache using a new `preserve_rendered_state` variable. * narrow_state.filter() returns the filter of combined feed view list instead of `undefined`. * We start fetching messages from the latest message on app load. * Messages in all messages data and Recent view are always synced. * If combined feed view list is not cached, we don't track it's last pointer, effectively sending user to the latest unread message always .
|
I think I'm good to merge this. Follow-ups would be:
Merged, huge thanks for all the work on this! I expect we'll be able to see the impact in our graphs for the next deployment. |
If the initial fetch pre zulip#29740 fetches all muted messages we don't have any messages in all message list and hence query for oldest message is undefined. This results in us trying to render oldest_message_timestamp with its value as infinity. So, to fix it, we just wait for our other initial fetch of recent view to return us the messages we need.
If the initial fetch pre zulip#29740 fetches all muted messages we don't have any messages in all message list and hence query for oldest message is undefined. This results in us trying to render oldest_message_timestamp with its value as infinity. So, to fix it, we just wait for our other initial fetch of recent view to return us the messages we need.
If the initial fetch pre zulip#29740 fetches all muted messages we don't have any messages in all message list and hence query for oldest message is undefined. This results in us trying to render oldest_message_timestamp with its value as infinity. To fix it, we include muted messages in our search for oldest message in the list and if we still cannot find one, we wait for the next fetch.
If the initial fetch pre zulip#29740 fetches all muted messages we don't have any messages in all message list and hence query for oldest message is undefined. This results in us trying to render oldest_message_timestamp with its value as infinity. To fix it, we include muted messages in our search for oldest message in the list and if we still cannot find one, we wait for the next fetch.
If the initial fetch pre zulip#29740 fetches all muted messages we don't have any messages in all message list and hence query for oldest message is undefined. This results in us trying to render oldest_message_timestamp with its value as infinity. To fix it, we include muted messages in our search for oldest message in the list and if we still cannot find one, we wait for the next fetch.
If the initial fetch pre #29740 fetches all muted messages we don't have any messages in all message list and hence query for oldest message is undefined. This results in us trying to render oldest_message_timestamp with its value as infinity. To fix it, we include muted messages in our search for oldest message in the list and if we still cannot find one, we wait for the next fetch.
Improved version of #29259
Discussion: https://chat.zulip.org/#narrow/stream/6-frontend/topic/fetching.20message.20history
TODO: Fix node tests.
Important changes in this commit:
We only cache message list for "All messages" if it is the default view.
We modify existing handling of home message list code so that it can be used to for any message list that we want to cache using a new
preserve_rendered_statevariable.narrow_state.filter() is return the filter for all messages list instead of
undefined.We start fetching messages from the latest message on app load.
Messages in all messages view and Recent view are always synced.
If all messages view list is not cached, we don't track it's last pointer, effectively sending user to the latest unread message always .