1546723 - LSNG: Use the WriteOptimizer on the child side too

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, enhancement, P1)

Description

[Jan Varga [:janv]]

See bug 1534222.

Comment 1

[Jan Varga [:janv]]

I'm finishing patches for this.

Comment 2

[Jan Varga [:janv]]

Attachment: Bug 1546723 - Part 1: Convert WriterOptimizer to a generic reusable class; r=asuth

This patch creates a new generic class LSWriteOptimizer which can be used with any value type for specific write optimizations either on the parent side or the child side.

Comment 3

[Jan Varga [:janv]]

Attachment: Bug 1546723 - Part 2: Make it more clear that checkpointing also notifies observers; r=asuth

This patch renames the Checkpoint IPC message to CheckpointAndNotify. Other structures used by checkpointing are renamed too. Datastore methods SetItem/RemoveItem/Clear no longer call NotifyObservers, it's now up to RecvCheckpointAndNotify to call it.

Comment 4

[Jan Varga [:janv]]

Attachment: Bug 1546723 - Part 3: Change GetSnapshotInitInfo to deal with load info only; r=asuth

This patch renames GetSnapshotInitInfo to GetSnapshotLoadInfo and removes some arguments that are not directly related to load info.

Comment 5

[Jan Varga [:janv]]

Attachment: Bug 1546723 - Part 4: Use a write optimizer in LSSnapshot; r=asuth

This patch adds a write optimizer to LSSnapshot. The optimizer is only used when
there are no observers for other content processes.

Comment 6

[Jan Varga [:janv]]

Attachment: Bug 1546723 - Part 5: Generalize code for getting datastores; r=asuth

Comment 7

[Jan Varga [:janv]]

Attachment: Bug 1546723 - Part 6: Mark snapshot as dirty if the hasOtherProcessObservers flag changes; r=asuth

Comment 8

[Jan Varga [:janv]]

Attachment: Bug 1546723 - Part 7: Fix ordering of items in special cases; r=asuth

Comment 9

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=740fb3af4ecb25334961a9c13866c46f4ca9423d

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb5651a75c2
Part 1: Convert WriterOptimizer to a generic reusable class; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/52d60f7805dc
Part 2: Make it more clear that checkpointing also notifies observers; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b79d3220097
Part 3: Change GetSnapshotInitInfo to deal with load info only; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/980ca798d66a
Part 4: Use a write optimizer in LSSnapshot; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/4402b4a26871
Part 5: Generalize code for getting datastores; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e420cc50e58
Part 6: Mark snapshot as dirty if the hasOtherProcessObservers flag changes; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d119de19f126
Part 7: Fix ordering of items in special cases; r=asuth

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b439d3fd8862
Followup fix: Convert MOZ_DIAGNOSTIC_ASSERT back to MOZ_ASSERT since bug 1541775 hasn't been fixed yet; r=asuth

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3bb5651a75c2
https://hg.mozilla.org/mozilla-central/rev/52d60f7805dc
https://hg.mozilla.org/mozilla-central/rev/1b79d3220097
https://hg.mozilla.org/mozilla-central/rev/980ca798d66a
https://hg.mozilla.org/mozilla-central/rev/4402b4a26871
https://hg.mozilla.org/mozilla-central/rev/1e420cc50e58
https://hg.mozilla.org/mozilla-central/rev/d119de19f126
https://hg.mozilla.org/mozilla-central/rev/b439d3fd8862

Comment 13

[Jan Varga [:janv]]

Comment on attachment 9064971 [details]
Bug 1546723 - Part 1: Convert WriterOptimizer to a generic reusable class; r=asuth

Beta/Release Uplift Approval Request

• User impact if declined: Users would still experience crashes reported in bug 1534222 (happens with LSNG only) and bug 1508740 (happens with the old LS implementation and LSNG too).
  In other words, if the fix is approved for beta, a long standing issue reported in bug 1508740 should be mitigated (if not fixed completely) on beta too.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): These changes are quite isolated and if there's a problem we can disable write optimizer with a simple patch or disabled LSNG completely.
• String changes made/needed: None

Comment 14

[Julien Cristau [:jcristau]]

Does this change affect the non-LSNG path at all? If so, how do we guard against regressions on that code path?

Comment 15

[Julien Cristau [:jcristau]]

Or I guess I was confused by the mention of old LS in comment 13, but this wouldn't fix it there, it fixes it for the LSNG case?

Comment 16

[Jan Varga [:janv]]

(In reply to Julien Cristau [:jcristau] from comment #14)

Does this change affect the non-LSNG path at all? If so, how do we guard against regressions on that code path?

Yes, it also affects the non-LSNG path. It fixes bug 1508740 (LSNG must be enabled).

Comment 17

[Jan Varga [:janv]]

(In reply to Julien Cristau [:jcristau] from comment #15)

Or I guess I was confused by the mention of old LS in comment 13, but this wouldn't fix it there, it fixes it for the LSNG case?

The set of patches in this bug fixes two bugs (crashes):

1. The crash reported in bug 1534222
   This crash happens only with LSNG enabled

2. The crash reported in bug 1508740
   This is a long standing crash that was not possible to fix with old LS implementation (non-LSNG implementation). So, LSNG must be enabled and also the write optimizer developed in this bug.

Comment 18

[Jan Varga [:janv]]

(In reply to Julien Cristau [:jcristau] from comment #14)

Does this change affect the non-LSNG path at all? If so, how do we guard against regressions on that code path?

If there are regressions, we can easily disable the write optimizer with a simple patch or in the worst case we can just disabled LSNG completely on beta.

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 9064971 [details]
Bug 1546723 - Part 1: Convert WriterOptimizer to a generic reusable class; r=asuth

localstorage fixes for 68.0b10.

Comment 20

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3660181b7bca
https://hg.mozilla.org/releases/mozilla-beta/rev/4b55e5f016e6
https://hg.mozilla.org/releases/mozilla-beta/rev/1521249d7a07
https://hg.mozilla.org/releases/mozilla-beta/rev/ac0e29e198c4
https://hg.mozilla.org/releases/mozilla-beta/rev/e23ee28c87d7
https://hg.mozilla.org/releases/mozilla-beta/rev/0a94dff558b1
https://hg.mozilla.org/releases/mozilla-beta/rev/ad7c72a799ba
https://hg.mozilla.org/releases/mozilla-beta/rev/2a42b81259f6