Skip to content

redux-persist: Stop saving REHYDRATE payload to disk.#4373

Merged
gnprice merged 1 commit intozulip:masterfrom
chrisbobbe:pr-stop-persisting-rehydrate-payload
Jan 1, 2021
Merged

redux-persist: Stop saving REHYDRATE payload to disk.#4373
gnprice merged 1 commit intozulip:masterfrom
chrisbobbe:pr-stop-persisting-rehydrate-payload

Conversation

@chrisbobbe
Copy link
Copy Markdown
Contributor

The expensive part of persisting the state to disk is running
stringify (see replaceRevive.js). While #4348 brought this time
from almost 2000ms down to, perhaps, 300ms on one account on my
device [1], that's still a long time, especially for work that
doesn't need to be done.

See discussion [2] for this solution.

[1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60remotedev-serialize.60.20and.20.60jsan.60/near/1083700
[2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60remotedev-serialize.60.20and.20.60jsan.60/near/1089466

@chrisbobbe chrisbobbe requested a review from gnprice December 31, 2020 16:13
@chrisbobbe

This comment has been minimized.

@chrisbobbe chrisbobbe force-pushed the pr-stop-persisting-rehydrate-payload branch from 149346f to e8cc6e9 Compare December 31, 2020 16:18
@chrisbobbe

This comment has been minimized.

@chrisbobbe
Copy link
Copy Markdown
Contributor Author

chrisbobbe commented Dec 31, 2020

Here's a chart for starting up the app (on a release build on one account on CZO on my iPhone) six times: before the change, and after.

image

Logging code:

diff --git src/boot/replaceRevive.js src/boot/replaceRevive.js
index 7c1eb0e9f..91eb92949 100644
--- src/boot/replaceRevive.js
+++ src/boot/replaceRevive.js
@@ -2,6 +2,7 @@
 import invariant from 'invariant';
 import Immutable from 'immutable';
 
+import * as logging from '../utils/logging';
 import { ZulipVersion } from '../utils/zulipVersion';
 import { GravatarURL, UploadedAvatarURL, FallbackAvatarURL } from '../utils/avatar';
 
@@ -147,7 +148,10 @@ const reviver = function reviver(key, value) {
 };
 
 export const stringify = function stringify(data: mixed): string {
+  const before = Date.now();
   const result = JSON.stringify(data, replacer);
+  const after = Date.now();
+  logging.logToProfiling({ durationMs: after - before, ip: '192.168.1.7' });
   if (result === undefined) {
     // Flow says that the output for JSON.stringify could be
     // undefined. From MDN:

Hmm, interesting! I see the expected result that there's just one tall bar per startup (that's the /register response), instead of two (REHYDRATE payload and the /register response). But I also see that the total number of stringify calls has been cut pretty much in half, and I don't yet understand why.

@chrisbobbe chrisbobbe force-pushed the pr-stop-persisting-rehydrate-payload branch from e8cc6e9 to 7389870 Compare December 31, 2020 16:43
@gnprice
Copy link
Copy Markdown
Member

gnprice commented Jan 1, 2021

Hmm, interesting! I see the expected result that there's just one tall bar per startup (that's the /register response), instead of two (REHYDRATE payload and the /register response). But I also see that the total number of stringify calls has been cut pretty much in half, and I don't yet understand why.

One thing is that when we've made updates across the state tree, that doesn't result in just one call to stringify -- each top-level subtree like state.messages, state.users, etc., is persisted separately, so will cause a separate call to stringify.

So the tall bars come from /register and rehydrate, but so do a lot of shorter bars. The tall bars are presumably the ones for state.users, since that's the giant part of the state.

The expensive part of persisting the state to disk is running
`stringify` (see replaceRevive.js). While zulip#4348 brought this time
from almost 2000ms down to, perhaps, 300ms on one account on my
device [1], that's still a long time, especially for work that
doesn't need to be done.

See discussion [2] for how we arrived at this solution.

[1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60remotedev-serialize.60.20and.20.60jsan.60/near/1083700
[2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60remotedev-serialize.60.20and.20.60jsan.60/near/1089466
@gnprice gnprice force-pushed the pr-stop-persisting-rehydrate-payload branch from 7389870 to 08b77bb Compare January 1, 2021 01:21
@gnprice gnprice merged commit 08b77bb into zulip:master Jan 1, 2021
@gnprice
Copy link
Copy Markdown
Member

gnprice commented Jan 1, 2021

Thanks -- merged! I made a tiny edit to add a blank line above each of the two new comments, to separate it visually from the code above.

@chrisbobbe
Copy link
Copy Markdown
Contributor Author

One thing is that when we've made updates across the state tree, that doesn't result in just one call to stringify -- each top-level subtree like state.messages, state.users, etc., is persisted separately, so will cause a separate call to stringify.

So the tall bars come from /register and rehydrate, but so do a lot of shorter bars. The tall bars are presumably the ones for state.users, since that's the giant part of the state.

Oh OK, right, that makes sense! Thanks for the review, tweak, and merge.

@chrisbobbe chrisbobbe deleted the pr-stop-persisting-rehydrate-payload branch November 4, 2021 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants