Bug 26629 – Refactor localStorage code for use in multi-process browsers (part 2)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Original bug: https://bugs.webkit.org/show_bug.cgi?id=25376

[Jeremy Orlow]

Created attachment 31688 [details]
patch v1 (corresponds to patch 2 - v2 in the other thread)

Simplify the interaction between LocalStorageArea/SessionStorageArea and
StorageArea by creating a "blockUntilImportComplete()" function rather than
bouncing back and forth between the child and parent classes in a somewhat
unintuitive manner.

This patch also paves the way for LocalStorageArea and SessionStorageArea being
merged into one.  It's part of several in a set which are separating syncing
(to disk) code from the rest of the implementation so that local storage and
session storage's code can be unified.

(Split out from https://bugs.webkit.org/show_bug.cgi?id=25376)

[Brady Eidson]

Comment on attachment 31688 [details]
patch v1 (corresponds to patch 2 - v2 in the other thread)

In general, a big task like this being split up into multiple patches is a great thing.  It helps follow the progression of the task and more easily digest each step.  I appreciate that you've done that.  

Admittedly I haven't looked ahead to patches 3/4/5 yet but that said...

> +// FIXME: In the future, we should allow use of localStorage while it's importing (when safe to do so).
> +// Blocking everything until the import is complete is by far the simplest and safest thing to do, but
> +// there is certainly room for safe optimization: Key/length will never be able to make use of such an
> +// optimization (since the order of iteration can change as items are being added). Get can return any
> +// item currently in the map. Get/remove can work whether or not it's in the map, but we'll need a list
> +// of items the import should not overwrite. Clear can also work, but it'll need to kill the import
> +// job first.

What this FIXME isn't saying is that most of the optimizations it refers to currently exist, but this patch is removing them.  I don't think it's okay to have that regression, even temporarily.

This patch seems fine on the surface, except for this regression.  Does 3/4/5 restore the optimization?

[Jeremy Orlow]

(In reply to comment #2)
> (From update of attachment 31688 [details] [review])
> In general, a big task like this being split up into multiple patches is a
> great thing.  It helps follow the progression of the task and more easily
> digest each step.  I appreciate that you've done that.  
> 
> Admittedly I haven't looked ahead to patches 3/4/5 yet but that said...
> 
> > +// FIXME: In the future, we should allow use of localStorage while it's importing (when safe to do so).
> > +// Blocking everything until the import is complete is by far the simplest and safest thing to do, but
> > +// there is certainly room for safe optimization: Key/length will never be able to make use of such an
> > +// optimization (since the order of iteration can change as items are being added). Get can return any
> > +// item currently in the map. Get/remove can work whether or not it's in the map, but we'll need a list
> > +// of items the import should not overwrite. Clear can also work, but it'll need to kill the import
> > +// job first.
> 
> What this FIXME isn't saying is that most of the optimizations it refers to
> currently exist, but this patch is removing them.  I don't think it's okay to
> have that regression, even temporarily.

I'm not sure I agree.

The locking semantics of length and key don't change with this patch.  Clear simply does no blocking/locking, which I believe was an oversight.

I believe setItem/removeItem are currently race-y since they only block on the import lock and not on m_importComplete.  Since there's so much code (like opening the db, reading it in, etc) that runs before taking the lock, it's quite easy for setItem/removeItem to run before the import.  The import can then overwrite changes.

So that simply leaves getItem.  In the current code, if the key/value has already been loaded when getItem is called, it'll return the item right away--even if the import isn't completed.  Unfortunately, this is (correctly) wrapped by a lock.  The import code holds this lock from the time it starts loading items into the map until it finishes.  So as far as I can see, there's never a case where this 'fast path' would help.

So I don't think this patch is removing any valid optimization and is fixing some issues in the locking semantics.

I guess I should have explained this better in the change description.  Sorry about that.

> This patch seems fine on the surface, except for this regression.  Does 3/4/5
> restore the optimization?

No, but I was planning on doing this _soon_ after basic functionality was working within Chromium.  And I expect that to be true soon after these patches land.  I'm actually pretty excited about doing that kind of work, but I can't put basic functionality (in Chromium) on hold while doing the fun stuff.

[Jeremy Orlow]

I've created https://bugs.webkit.org/show_bug.cgi?id=26636 to track the need to reliably test race conditions in code like this.

I'm also changing the title of this bug to be more descriptive.

[Brady Eidson]

I see now that the expected optimization with set/remove/clear was never actually there.  The code was complicated like this for that very reason.

getItem() was the one I actually already knew there was a flaw with  ;)

My concern stands in a new form - changing the fine grain code to a big heavy block(), it seems to me the fine grained code will just have to be re-introduced at some point to get closer to the original intended optimizations.

Since they aren't in place, though, I guess the patch is fine.

[Jeremy Orlow]

(In reply to comment #5)
> I see now that the expected optimization with set/remove/clear was never
> actually there.  The code was complicated like this for that very reason.
> 
> getItem() was the one I actually already knew there was a flaw with  ;)
> 
> My concern stands in a new form - changing the fine grain code to a big heavy
> block(), it seems to me the fine grained code will just have to be
> re-introduced at some point to get closer to the original intended
> optimizations.

I agree.  Originally I tried to leave it in, but the patches were much uglier.  I figured since we weren't actually losing functionality it was OK, but my guilt is why I put that huge FIXME in.  I'll create a bug for it now.  :-)

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/44997