1136857 - Try to make DOMStorageCache more stable

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Honza Bambas (:mayhemer)]

Could fix bug 1013329 and bug 1108777 with some luck.

Comment 1

[Honza Bambas (:mayhemer)]

I think the mLoaded flag is OK.  Set under the lock, and protected by function boundaries where read unprotected.

I suspect any crashes caused by DOMStorage code may be coming from sqlite.  Under some circumstances we are doing a concurrent main thread select from the database.  We can switch that off (no need to have it anyway - only small rare performance hit might occur.)


Anyway, 

bug 1013329 is WFM.

bug 1108777 might be the same case.

So, we may be OK here and this may just turn to a "check how it is -> it's OK" bug that is also WFM.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Honza Bambas (:mayhemer) from comment #1)
> I think the mLoaded flag is OK.  Set under the lock, and protected by
> function boundaries where read unprotected.

FWIW, after bug 1108777 got discussed, I ran localstorage tests under TSan and it's turning up quite a few races, including one that appears to be on mLoaded.

Comment 3

[Nathan Froyd [:froydnj]]

(I'm going to file bugs after I have a chance to analyze the race reports a bit.)

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3)
> (I'm going to file bugs after I have a chance to analyze the race reports a
> bit.)

Thanks!  (I'm falling in love with TSan ;))

Comment 5

[Honza Bambas (:mayhemer)]

Jan, do you think you could check on the depending bugs here and find out if those would go away with your QM work?  If not, could you take care of them (or find an assignee)?

Comment 6

[Honza Bambas (:mayhemer)]

No response from Jav Varga for more then 3 weeks.  I think I'll try a very simple solution here.

Based on bug 1136922 comment 4 I think the solution here is to simply make mLoaded an atomic.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v1

Using release-acquire since this is just a signaling flag.  But fully sequential might be safer here?  Not sure about the performance benefits here.

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

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8713614 [details] [diff] [review]
v1

Pressed enter too soon...

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1

based on bug 1136922 comment 1 - 4.

- using rel-acq since this is a status flag only
- not sure this will make TSan happy tho.. however, now I'm confident access to mData is protected well enough to blacklist it from TSan's checks (not part of this patch as I need a confirmation on this first)

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

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8713642 [details] [diff] [review]
v1

Review of attachment 8713642 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, I suppose, assuming we're only touching mLoaded on two threads and not more.

::: dom/storage/DOMStorageCache.cpp
@@ +565,5 @@
>  DOMStorageCache::CloneFrom(const DOMStorageCache* aThat)
>  {
> +  // This will never be called on anything else than SessionStorage.
> +  // This means mData will never be touched on any other thread than
> +  // the main thread and it never went through the loading process.

Why not just assert NS_IsMainThread here, since we're touching mData below?

(I'm not really qualified to review this part of the patch, so apologies in advance of dumb questions...)

::: dom/storage/DOMStorageCache.h
@@ +222,5 @@
>    // the database (i.e. it is persistent) this flags is set to true after
>    // all keys and coresponding values are loaded from the database.
> +  // This flag never goes from true back to false.  Since this flag is
> +  // critical for mData hashtable synchronization, it's made atomic.
> +  Atomic<bool, ReleaseAcquire> mLoaded;

I think ReleaseAcquire is sufficient here (this is used for synchronization between only two threads, yes?), though the default would be better unless we have good reason to think this is very hot code...

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Nathan Froyd [:froydnj] from comment #14)
> Comment on attachment 8713642 [details] [diff] [review]
> v1
> 
> Review of attachment 8713642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, I suppose, assuming we're only touching mLoaded on two threads and not
> more.

Thanks for a quick r!

Only two threads so far.  I will watch how any planned IPC changes go on, those might introduce another thread accessing mData[] and mLoaded.

> 
> ::: dom/storage/DOMStorageCache.cpp
> @@ +565,5 @@
> >  DOMStorageCache::CloneFrom(const DOMStorageCache* aThat)
> >  {
> > +  // This will never be called on anything else than SessionStorage.
> > +  // This means mData will never be touched on any other thread than
> > +  // the main thread and it never went through the loading process.
> 
> Why not just assert NS_IsMainThread here, since we're touching mData below?

We can do that, yes.

> 
> (I'm not really qualified to review this part of the patch, so apologies in
> advance of dumb questions...)
> 
> ::: dom/storage/DOMStorageCache.h
> @@ +222,5 @@
> >    // the database (i.e. it is persistent) this flags is set to true after
> >    // all keys and coresponding values are loaded from the database.
> > +  // This flag never goes from true back to false.  Since this flag is
> > +  // critical for mData hashtable synchronization, it's made atomic.
> > +  Atomic<bool, ReleaseAcquire> mLoaded;
> 
> I think ReleaseAcquire is sufficient here (this is used for synchronization
> between only two threads, yes?), though the default would be better unless
> we have good reason to think this is very hot code...

It's definitely not a hot code.  I can change it, but we sync only between two threads and I would like to save the benefits of not having a mutex on the main thread here.  I don't know the difference of rel/acq vs seq well enough to see any performance implications, so hard to decide.

Comment 16

[Jan Varga [:janv]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Jan, do you think you could check on the depending bugs here and find out if
> those would go away with your QM work?  If not, could you take care of them
> (or find an assignee)?

Well, we're still in early stage with QM work, sorry.

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Jan Varga [:janv] from comment #16)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > Jan, do you think you could check on the depending bugs here and find out if
> > those would go away with your QM work?  If not, could you take care of them
> > (or find an assignee)?
> 
> Well, we're still in early stage with QM work, sorry.

I've fixed this.

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v1.1

- added main thread assertion in clone
- mem ordering left at rel/acq

Comment 19

[Honza Bambas (:mayhemer)]

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7d83c65f5d

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea7d83c65f5d