Bug 26629

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)

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?

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

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.

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.

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

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