serialize: Comment out handlers for Immutable things Zulip doesn't use.#2
Open
chrisbobbe wants to merge 2 commits intozulip:masterfrom
Open
serialize: Comment out handlers for Immutable things Zulip doesn't use.#2chrisbobbe wants to merge 2 commits intozulip:masterfrom
chrisbobbe wants to merge 2 commits intozulip:masterfrom
Conversation
One can always read the readme at `zalmoxisus/remotedev-serialize`, and this makes it faster to develop our fork. In particular, we won't have to keep the `Supported` list up to date as we change the list of Immutable data structures that our fork handles. (We'll go straight to the code for that; if we get something wrong, a stack trace will be more helpful than a README.)
This has been observed to save >20% of the time it takes to persist our Redux storage to disk [1]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1082087
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 18, 2020
It turns out that remotedev-serialize is doing some tasks that we don't need it to do. See discussion for more background [1]. The changes we'd like to make to fix that will reduce the amount of code used to implement our replace/revive logic, and it'll make sense to just fold remotedev-serialize into our codebase instead of depending on it as a separate module. One particular thing we want to do is cut `jsan`, something used by remotedev-serialize, out of the picture. One main feature `jsan` announces is that it can handle serializing an object with circular references -- but we clearly don't need this, since our data is naturally free of them. And as it checks for circular references, it makes a deep copy of the whole data structure. We also don't need any of the other fancy features `jsan` advertises, like persisting functions. [2] So we can potentially gain a good performance boost by just cutting `jsan` out of the picture. We'll do that after all these "Inline remotedev-serialize (n/x)" commits. Another thing (and I opened this as zulip/remotedev-serialize#2 before we decided to inline remotedev-serialize), we've observed [3] that we can save maybe >20% of the time we periodically spend persisting the state by removing various lines in remotedev-serialize itself (not `jsan`) that handle Immutable.Map data structures that we don't currently use. Start by cutting out the `Serialize.immutable` factory and just writing out `stringify` and `parse` (previously obtained by calling `Serialize.immutable`) ourselves, based on `Serialize.immutable`'s implementation and the inputs we'd been giving it. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60remotedev-serialize.60.20and.20.60jsan.60/near/1082744 [2] It turns out that the circular reference-handling code does helpfully recurse through and apply our custom replacer function on values before they've been transformed by their `toJSON` methods. We'll handle that wrinkle when we take `jsan` out, a few commits from now. [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1082087
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 18, 2020
This has been observed to save >20% of the time it takes to persist our Redux storage to disk [1]. It's also convenient for the mechanics of inlining remotedev-serialize; e.g., we won't have to spell out `refer` at all, since it's not used after this commit. Originally opened as zulip/remotedev-serialize#2, before it became clear that we'd like to inline remotedev-serialize. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1082087
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 24, 2020
It turns out that remotedev-serialize is doing some tasks that we don't need it to do. See discussion for more background [1]. The changes we'd like to make to fix that will reduce the amount of code used to implement our replace/revive logic, and it'll make sense to just fold remotedev-serialize into our codebase instead of depending on it as a separate module. One particular thing we want to do is cut `jsan`, something used by remotedev-serialize, out of the picture. One main feature `jsan` announces is that it can handle serializing an object with circular references -- but we clearly don't need this, since our data is naturally free of them. And as it checks for circular references, it makes a deep copy of the whole data structure. We also don't need any of the other fancy features `jsan` advertises, like persisting functions. [2] So we can potentially gain a good performance boost by just cutting `jsan` out of the picture. We'll do that after all these "Inline remotedev-serialize (n/x)" commits. Another thing (and I opened this as zulip/remotedev-serialize#2 before we decided to inline remotedev-serialize), we've observed [3] that we can save maybe >20% of the time we periodically spend persisting the state by removing various lines in remotedev-serialize itself (not `jsan`) that handle Immutable.Map data structures that we don't currently use. Start by cutting out the `Serialize.immutable` factory and just writing out `stringify` and `parse` (previously obtained by calling `Serialize.immutable`) ourselves, based on `Serialize.immutable`'s implementation and the inputs we'd been giving it. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60remotedev-serialize.60.20and.20.60jsan.60/near/1082744 [2] It turns out that the circular reference-handling code does helpfully recurse through and apply our custom replacer function on values before they've been transformed by their `toJSON` methods. We'll handle that wrinkle when we take `jsan` out, a few commits from now. [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1082087
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 24, 2020
This has been observed to save >20% of the time it takes to persist our Redux storage to disk [1]. It's also convenient for the mechanics of inlining remotedev-serialize; e.g., we won't have to spell out `refer` at all, since it's not used after this commit. Originally opened as zulip/remotedev-serialize#2, before it became clear that we'd like to inline remotedev-serialize. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1082087
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 28, 2020
It turns out that remotedev-serialize is doing some tasks that we don't need it to do. See discussion for more background [1]. The changes we'd like to make to fix that will reduce the amount of code used to implement our replace/revive logic, and it'll make sense to just fold remotedev-serialize into our codebase instead of depending on it as a separate module. One particular thing we want to do is cut `jsan`, something used by remotedev-serialize, out of the picture. One main feature `jsan` announces is that it can handle serializing an object with circular references -- but we clearly don't need this, since our data is naturally free of them. And as it checks for circular references, it makes a deep copy of the whole data structure. We also don't need any of the other fancy features `jsan` advertises, like persisting functions. [2] So we can potentially gain a good performance boost by just cutting `jsan` out of the picture. We'll do that after all these "Inline remotedev-serialize (n/x)" commits. Another thing (and I opened this as zulip/remotedev-serialize#2 before we decided to inline remotedev-serialize), we've observed [3] that we can save maybe >20% of the time we periodically spend persisting the state by removing various lines in remotedev-serialize itself (not `jsan`) that handle Immutable.Map data structures that we don't currently use. Start by cutting out the `Serialize.immutable` factory and just writing out `stringify` and `parse` (previously obtained by calling `Serialize.immutable`) ourselves, based on `Serialize.immutable`'s implementation and the inputs we'd been giving it. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60remotedev-serialize.60.20and.20.60jsan.60/near/1082744 [2] It turns out that the circular reference-handling code does helpfully recurse through and apply our custom replacer function on values before they've been transformed by their `toJSON` methods. We'll handle that wrinkle when we take `jsan` out, a few commits from now. [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1082087
chrisbobbe
added a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 28, 2020
This has been observed to save >20% of the time it takes to persist our Redux storage to disk [1]. It's also convenient for the mechanics of inlining remotedev-serialize; e.g., we won't have to spell out `refer` at all, since it's not used after this commit. Originally opened as zulip/remotedev-serialize#2, before it became clear that we'd like to inline remotedev-serialize. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1082087
gnprice
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 29, 2020
It turns out that remotedev-serialize is doing some tasks that we don't need it to do. See discussion for more background [1]. The changes we'd like to make to fix that will reduce the amount of code used to implement our replace/revive logic, and it'll make sense to just fold remotedev-serialize into our codebase instead of depending on it as a separate module. One particular thing we want to do is cut `jsan`, something used by remotedev-serialize, out of the picture. One main feature `jsan` announces is that it can handle serializing an object with circular references -- but we clearly don't need this, since our data is naturally free of them. And as it checks for circular references, it makes a deep copy of the whole data structure. We also don't need any of the other fancy features `jsan` advertises, like persisting functions. [2] So we can potentially gain a good performance boost by just cutting `jsan` out of the picture. We'll do that after all these "Inline remotedev-serialize (n/x)" commits. Another thing (and I opened this as zulip/remotedev-serialize#2 before we decided to inline remotedev-serialize), we've observed [3] that we can save maybe >20% of the time we periodically spend persisting the state by removing various lines in remotedev-serialize itself (not `jsan`) that handle Immutable.Map data structures that we don't currently use. Start by cutting out the `Serialize.immutable` factory and just writing out `stringify` and `parse` (previously obtained by calling `Serialize.immutable`) ourselves, based on `Serialize.immutable`'s implementation and the inputs we'd been giving it. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60remotedev-serialize.60.20and.20.60jsan.60/near/1082744 [2] It turns out that the circular reference-handling code does helpfully recurse through and apply our custom replacer function on values before they've been transformed by their `toJSON` methods. We'll handle that wrinkle when we take `jsan` out, a few commits from now. [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1082087
gnprice
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Dec 29, 2020
This has been observed to save >20% of the time it takes to persist our Redux storage to disk [1]. It's also convenient for the mechanics of inlining remotedev-serialize; e.g., we won't have to spell out `refer` at all, since it's not used after this commit. Originally opened as zulip/remotedev-serialize#2, before it became clear that we'd like to inline remotedev-serialize. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1082087
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NOTE: Please see zulip/zulip-mobile#4348 (which does the same work done here) before reviewing this—we might be removing
remotedev-serializeas a dependency.This has been observed to save >20% of the time it takes to persist
our Redux storage to disk [1].
[1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/user_avatar_url_field_optional/near/1082087