[Webkit-unassigned] [Bug 26629] Refactor localStorage code for use in multi-process browsers (part 2)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=26629





------- Comment #3 from jorlow at chromium.org  2009-06-22 17:32 PDT -------
(In reply to comment #2)
> (From update of attachment 31688 [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.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.