Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

SessionStorage: Write state as stream to disk instead of building model in memory first. #8358

Merged
merged 2 commits into from
Sep 11, 2020

Conversation

pocmo
Copy link
Contributor

@pocmo pocmo commented Sep 9, 2020

With this change we are using JsonWriter to write BrowserState directly to disk using a stream. Previously we first build a JSONObject tree in memory. This essentially duplicates the state and that could become big if a user had a lot of tabs open and not much RAM.

@pocmo pocmo added the 🕵️‍♀️ needs review PRs that need to be reviewed label Sep 9, 2020
@Amejia481 Amejia481 self-requested a review September 9, 2020 17:47
@Amejia481 Amejia481 self-assigned this Sep 9, 2020
@@ -18,5 +19,14 @@ interface EngineSessionState {
* When reading JSON from disk [Engine.createSessionState] can be used to turn it back into an [EngineSessionState]
* instance.
*/
@Deprecated("Use writeTo() instead.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Should we also include when this will be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to have a fixed date for removing this since we first need time to migrate some of our own components code over. My primary goal was to avoid any new code using this. :)
But I filed a new issue for eventually migrating everything and will also link this in a comment: #8370

Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@pocmo
Copy link
Contributor Author

pocmo commented Sep 11, 2020

bors r=Amejia481

@pocmo
Copy link
Contributor Author

pocmo commented Sep 11, 2020

bors r-

@bors
Copy link

bors bot commented Sep 11, 2020

Canceled.

@pocmo
Copy link
Contributor Author

pocmo commented Sep 11, 2020

bors r=Amejia481

bors bot pushed a commit that referenced this pull request Sep 11, 2020
8358: SessionStorage: Write state as stream to disk instead of building model in memory first. r=Amejia481 a=pocmo

With this change we are using `JsonWriter` to write `BrowserState` directly to disk using a stream. Previously we first build a `JSONObject` tree in memory. This essentially duplicates the state and that could become big if a user had a lot of tabs open and not much RAM.

Co-authored-by: Sebastian Kaspari <s.kaspari@gmail.com>
@bors
Copy link

bors bot commented Sep 11, 2020

Build failed:

@pocmo
Copy link
Contributor Author

pocmo commented Sep 11, 2020

bors retry

bors bot pushed a commit that referenced this pull request Sep 11, 2020
8358: SessionStorage: Write state as stream to disk instead of building model in memory first. r=Amejia481 a=pocmo

With this change we are using `JsonWriter` to write `BrowserState` directly to disk using a stream. Previously we first build a `JSONObject` tree in memory. This essentially duplicates the state and that could become big if a user had a lot of tabs open and not much RAM.

Co-authored-by: Sebastian Kaspari <s.kaspari@gmail.com>
@bors
Copy link

bors bot commented Sep 11, 2020

Build failed:

…disk instead of building model in memory first.
@pocmo
Copy link
Contributor Author

pocmo commented Sep 11, 2020

bors retry

@bors
Copy link

bors bot commented Sep 11, 2020

Build succeeded:

@bors bors bot merged commit be5ef0b into mozilla-mobile:master Sep 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🕵️‍♀️ needs review PRs that need to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants