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

Consistently clear old data on all ways of leaving an account #4446

Closed
gnprice opened this issue Jan 29, 2021 · 3 comments · Fixed by #5606 or #5612
Closed

Consistently clear old data on all ways of leaving an account #4446

gnprice opened this issue Jan 29, 2021 · 3 comments · Fixed by #5606 or #5612
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness a-multi-org a-onboarding Everything you would do when first joining a realm. P1 high-priority

Comments

@gnprice
Copy link
Member

gnprice commented Jan 29, 2021

Most of our reducers reset their state when they get one of the action types LOGOUT or ACCOUNT_SWITCH. Many of them treat LOGIN_SUCCESS the same way; but many others don't.

We should make a consistent pattern here, document it, and ideally refactor so that the pattern gets maintained naturally.

Here are some notes, based on #3213 (comment) which I wrote a couple of years ago (but updated):

  • The three action types LOGOUT, ACCOUNT_SWITCH, LOGIN_SUCCESS don't appear in any sequence -- they correspond to completely independent actions in the UI, namely the "Logout" button; choosing an already-authenticated account in the account picker screen; and authenticating to an account we didn't have a key for.

    • I think this may mean there are live bugs here: if you're using one account, and then you go log into a new account, the various reducers that don't respond to LOGIN_SUCCESS won't reset the data.
      • They should discard the old data a bit later, at least, when they get a REALM_INIT action with the data from the new initial fetch.
      • I'm not sure this causes any live bugs, though, because it looks like we navigate to the loading screen in the loginSuccess action creator. So that may make the fact we haven't discarded the old data invisible, and then we should only leave the loading screen upon REALM_INIT.
  • The action creators for those three action types all look like this -- there's a thunk action which triggers a navigation, and then dispatches a plain-old-object action:

const logoutPlain = (): Action => ({
  type: LOGOUT,
});

export const logout = () => async (dispatch: Dispatch, getState: GetState) => {
  NavigationService.dispatch(resetToAccountPicker());
  dispatch(logoutPlain());
};
  • I think it'd be a useful refactor to consolidate one action type like CLEAR_ACCOUNT_DATA.
    • The action creators for these three (in accountActions.js) can dispatch that first, then also a LOGOUT etc.
    • Almost all the reducers would respond only to the CLEAR_ACCOUNT_DATA.
    • There are a couple of exceptions, like accountReducers and sessionReducers, that actually want to treat the three cases differently. Those would be the only ones to continue responding to LOGOUT etc.; and they'd stand out better as a result.
    • Then there are further changes we might make to those, but that'll be easier to see after that.
@gnprice gnprice added a-multi-org a-onboarding Everything you would do when first joining a realm. a-data-sync Zulip's event system, event queues, staleness/liveness P1 high-priority labels Jan 29, 2021
@gnprice
Copy link
Member Author

gnprice commented Jan 29, 2021

See also #3822 for a related improvement we made more recently.

gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jan 30, 2021
This makes us quite a bit faster at handling a message being marked
as read!  That's a frequent and latency-sensitive path -- whenever
the user is reading a conversation and sees some messages that were
unread, this is part of the path to updating the "N unreads" banner
at the top of their view.  Measurements described below.

As we convert the other parts of the unread state, we'll want to fold
their reducers into this file too, and in fact combine the logic.  No
need to have four copies of all this, most of which is the same.

While making this conversion, I notice that this reducer doesn't reset
state on LOGIN_SUCCESS like it does for LOGOUT and ACCOUNT_SWITCH.  In
general we're pretty consistent about resetting state on those latter
two, but many of our reducers do so on LOGIN_SUCCESS while many others
don't.  I've filed zulip#4446 for fixing them all up to be consistent.

Performance measurements:

I made some manual performance measurements to evaluate this change
and the others in this series.  I used a test user with lots of
unreads on chat.zulip.org, on a few-years-old flagship phone: a
Pixel 2 XL running Android 10.  The test user has 50k unreads in this
data structure (that's the max the server will send in the initial
fetch), across about 3400 topics in 27 different streams.

Before this series, on visiting a topic with 1 unread, we'd spend
about 70ms in this reducer, which is a long time.  We'd spend 300ms in
total on dispatching the EVENT_UPDATE_MESSAGE_FLAGS action, including
the time spent in the reducer.  (The other time is probably some
combination of React re-rendering the components that use this data;
before that, our selectors that sit between those components and this
data recomputing their own results; and after that, React Native
applying the resulting updates to the underlying native components.
We don't yet have clear measurements to tell how much time those each
contribute.)

After this change, we spend about 30-50ms in the reducer, and a total
of 150-200ms in dispatch.  Still slow, but much improved!  We'll speed
this up further in upcoming commits.

For EVENT_NEW_MESSAGE, which is the other frequent update to this data
structure, not much changes: it was already "only" 4-9ms spent in this
reducer, which is too slow but far from our worst performance problem.
After this change, it's usually <=1ms (too small to measure with our
existing tools), and the longest I've seen is 3ms.  The total dispatch
time varies widely, like 70-200ms, and it's not clear if it changed.

There is one performance regression: we now spend about 100ms here on
REALM_INIT, i.e. on handling the data from the initial fetch.
Previously that time was <=1ms; we just took the data straight off the
wire (well, the data we'd already deserialized from the JSON that came
off the wire), and now we have to copy it into our more
efficiently-usable data structure.  As is, that 100ms is already well
worth it: we save more than 100ms, of visible latency, every time the
user reads some unread messages.  But it's enough to be worth
optimizing too, and we'll do so later in this series.

Fixes-partly: zulip#4438
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Mar 18, 2021
This makes us quite a bit faster at handling a message being marked
as read!  That's a frequent and latency-sensitive path -- whenever
the user is reading a conversation and sees some messages that were
unread, this is part of the path to updating the "N unreads" banner
at the top of their view.  Measurements described below.

As we convert the other parts of the unread state, we'll want to fold
their reducers into this file too, and in fact combine the logic.  No
need to have four copies of all this, most of which is the same.

While making this conversion, I notice that this reducer doesn't reset
state on LOGIN_SUCCESS like it does for LOGOUT and ACCOUNT_SWITCH.  In
general we're pretty consistent about resetting state on those latter
two, but many of our reducers do so on LOGIN_SUCCESS while many others
don't.  I've filed zulip#4446 for fixing them all up to be consistent.

Performance measurements:

I made some manual performance measurements to evaluate this change
and the others in this series.  I used a test user with lots of
unreads on chat.zulip.org, on a few-years-old flagship phone: a
Pixel 2 XL running Android 10.  The test user has 50k unreads in this
data structure (that's the max the server will send in the initial
fetch), across about 3400 topics in 27 different streams.

Before this series, on visiting a topic with 1 unread, we'd spend
about 70ms in this reducer, which is a long time.  We'd spend 300ms in
total on dispatching the EVENT_UPDATE_MESSAGE_FLAGS action, including
the time spent in the reducer.  (The other time is probably some
combination of React re-rendering the components that use this data;
before that, our selectors that sit between those components and this
data recomputing their own results; and after that, React Native
applying the resulting updates to the underlying native components.
We don't yet have clear measurements to tell how much time those each
contribute.)

After this change, we spend about 30-50ms in the reducer, and a total
of 150-200ms in dispatch.  Still slow, but much improved!  We'll speed
this up further in upcoming commits.

For EVENT_NEW_MESSAGE, which is the other frequent update to this data
structure, not much changes: it was already "only" 4-9ms spent in this
reducer, which is too slow but far from our worst performance problem.
After this change, it's usually <=1ms (too small to measure with our
tools, until recently), and the longest I've seen is 3ms.  The total
dispatch time varies widely, like 70-200ms; not clear if it changed.

There is one performance regression: we now spend about 100ms here on
REALM_INIT, i.e. on handling the data from the initial fetch.
Previously that time was <=1ms; we just took the data straight off the
wire (well, the data we'd already deserialized from the JSON that came
off the wire), and now we have to copy it into our more
efficiently-usable data structure.  As is, that 100ms is already well
worth it: we save more than 100ms, of visible latency, every time the
user reads some unread messages.  But it's enough to be worth
optimizing too, and we'll do so later in this series.

Fixes-partly: zulip#4438
@gnprice
Copy link
Member Author

gnprice commented Oct 6, 2022

Another issue with a similar flavor:

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 13, 2022
Most of these state subtrees hold per-account data we get from the
server. In the current pre-zulip#5005 design, where we only store server
data for one account, these should already have been clearing out on
LOGIN_SUCCESS, like we do with messages, narrows, etc.

Two of the subtrees, `outbox` and `drafts`, hold client-side
per-account data. Just as with server data, we've only been keeping
these around for the active account, and like the other reducers
touched in this commit, we've forgotten to handle LOGIN_SUCCESS for
them. So, do that.

Now, all the server-data subtrees and `outbox` and `drafts` all
reset on ACCOUNT_SWITCH, LOGIN_SUCCESS, and LOGOUT. That's part of
Greg's proposed fix for zulip#4446.

Next, we'll do the next part:

> * I think it'd be a useful refactor to consolidate one action type
>   like `CLEAR_ACCOUNT_DATA`.
>   * The action creators for these three (in `accountActions.js`) can
>     dispatch that first, then also a `LOGOUT` etc.
>   * Almost all the reducers would respond only to the
>     `CLEAR_ACCOUNT_DATA`.
>   * There are a couple of exceptions, like `accountReducers` and
>     `sessionReducers`, that actually want to treat the three cases
>     differently. Those would be the only ones to continue responding
>     to `LOGOUT` etc.; and they'd stand out better as a result.
>   * Then there are further changes we might make to those, but
>     that'll be easier to see after that.

Fixes-partly: zulip#4446
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 13, 2022
As Greg proposed in zulip#4446, so the pattern of consistently clearing
data on all ways of leaving an account gets maintained naturally:

(except we chose the name RESET_ACCOUNT_DATA instead of
CLEAR_ACCOUNT_DATA)

> * I think it'd be a useful refactor to consolidate one action type
>   like `CLEAR_ACCOUNT_DATA`.
>   * The action creators for these three (in `accountActions.js`) can
>     dispatch that first, then also a `LOGOUT` etc.
>   * Almost all the reducers would respond only to the
>     `CLEAR_ACCOUNT_DATA`.
>   * There are a couple of exceptions, like `accountReducers` and
>     `sessionReducers`, that actually want to treat the three cases
>     differently. Those would be the only ones to continue responding
>     to `LOGOUT` etc.; and they'd stand out better as a result.
>   * Then there are further changes we might make to those, but
>     that'll be easier to see after that.

Fixes: zulip#4446
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 14, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops.

Related: zulip#4446
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 14, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 14, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops.

Related: zulip#4446
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 14, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops.

Related: zulip#4446
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 14, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 14, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops.

Related: zulip#4446
@chrisbobbe
Copy link
Contributor

Reopening because I missed a spot or two in my #5606; see #5612.

@chrisbobbe chrisbobbe reopened this Dec 14, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 21, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops.

Related: zulip#4446
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 21, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 21, 2022
Really an instance of zulip#4446 that I forgot about in zulip#5606, oops.

Related: zulip#4446
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-data-sync Zulip's event system, event queues, staleness/liveness a-multi-org a-onboarding Everything you would do when first joining a realm. P1 high-priority
Projects
None yet
2 participants