Closed Bug 1546723 Opened 8 months ago Closed 6 months ago

LSNG: Use the WriteOptimizer on the child side too

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: janv, Assigned: janv)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Blocks: 1540402, 1508740
Type: defect → task
Assignee: nobody → jvarga
Priority: -- → P1
Type: task → enhancement

I'm finishing patches for this.

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.

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.

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

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

Attachment #9064976 - Attachment description: Bug 1546723 - Part 6: Mark snapshot as dirty if the hasObservers flag changes; r=asuth → Bug 1546723 - Part 6: Mark snapshot as dirty if the hasOtherProcessObservers flag changes; r=asuth
Pushed by jvarga@mozilla.com:
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
Pushed by jvarga@mozilla.com:
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
Blocks: 1539835

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
Attachment #9064971 - Flags: approval-mozilla-beta?
Attachment #9064972 - Flags: approval-mozilla-beta?
Attachment #9064973 - Flags: approval-mozilla-beta?
Attachment #9064974 - Flags: approval-mozilla-beta?
Attachment #9064975 - Flags: approval-mozilla-beta?
Attachment #9064976 - Flags: approval-mozilla-beta?
Attachment #9064977 - Flags: approval-mozilla-beta?

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

Flags: needinfo?(jvarga)

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?

(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).

Flags: needinfo?(jvarga)

(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.

(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 on attachment 9064971 [details]
Bug 1546723 - Part 1: Convert WriterOptimizer to a generic reusable class; r=asuth

localstorage fixes for 68.0b10.

Attachment #9064971 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9064972 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9064973 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9064974 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9064975 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9064976 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9064977 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.