Bug 28019

Created attachment 34135 [details] the patch This patch is actually a single-thread solution. Probably PLATFORM(WINCE) should be replaced with USE(XXXX_SINGLE_THREAD)? ENABLE(JSC_MULTIPLE_THREADS) seems not suitable here, because they are not JSC files.

Comment on attachment 34135 [details] the patch Should we use WINCE or a special USE(SINGLETHREADED_STORAGE)?

(In reply to comment #2) > (From update of attachment 34135 [details]) > Should we use WINCE or a special USE(SINGLETHREADED_STORAGE)? That's my question, too :) See above

(In reply to comment #2) > (From update of attachment 34135 [details]) > Should we use WINCE or a special USE(SINGLETHREADED_STORAGE)? WebCore/storage/wince has already been committed. If we change to USE(SINGLETHREADED_STORAGE), then we also should change WebCore/storage/wince to WebCore/storage/singlethreaded, and also change the file names.

(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 34135 [details] [details]) > > Should we use WINCE or a special USE(SINGLETHREADED_STORAGE)? > > WebCore/storage/wince has already been committed. > > If we change to USE(SINGLETHREADED_STORAGE), then we also should change > WebCore/storage/wince to WebCore/storage/singlethreaded, and also change the > file names. Well let's see if anyone else actually wants this feature, otherwise I see no reason to bother changing it just yet.

Comment on attachment 34135 [details] the patch This seems like the wrong approach. Please engage some of the other Database hackers and get them to make positive comments on the bug first before proceeding.

(In reply to comment #6) > (From update of attachment 34135 [details]) > This seems like the wrong approach. Please engage some of the other Database > hackers and get them to make positive comments on the bug first before > proceeding. Any more information about why it's wrong approach?

Comment on attachment 34135 [details] the patch Well, I don't know what this change is doing from the ChangeLog. I assume you're trying to make it possible to run Database operations on the main thread? Redirecting to a platform-specific header is generally never the right approach: #if PLATFORM(WINCE) 35 36 #include "wince/DatabaseThreadWince.h" 37 38 #else The abstraction (whatever you're trying to do there) likely belongs at a different/higher level. It would be bad, for example if 5 ports all had redirects there.

(In reply to comment #8) > (From update of attachment 34135 [details]) > Well, I don't know what this change is doing from the ChangeLog. I assume > you're trying to make it possible to run Database operations on the main > thread? > > Redirecting to a platform-specific header is generally never the right > approach: > #if PLATFORM(WINCE) > 35 > 36 #include "wince/DatabaseThreadWince.h" > 37 > 38 #else > > The abstraction (whatever you're trying to do there) likely belongs at a > different/higher level. It would be bad, for example if 5 ports all had > redirects there. Your assumption is correct. We are of the understanding that few if any will actually want this. If it's desired by others, I agree it should be refactored. In KDE we had a policy that something doesn't become an API until at least 2 separate users of it exist, otherwise it's a special case. I think that's prudent for WebKit too. I don't see the reason to burden the main code until we have evidence that someone else wants to use it too. Quite honestly, it's a bit ugly to have a multithreaded and a singlethreaded version in the same place so I'd rather bury this ugly one away.

(In reply to comment #8) > (From update of attachment 34135 [details]) > Well, I don't know what this change is doing from the ChangeLog. I assume > you're trying to make it possible to run Database operations on the main > thread? > > Redirecting to a platform-specific header is generally never the right > approach: > #if PLATFORM(WINCE) > 35 > 36 #include "wince/DatabaseThreadWince.h" > 37 > 38 #else > > The abstraction (whatever you're trying to do there) likely belongs at a > different/higher level. It would be bad, for example if 5 ports all had > redirects there. Platform-independent webkit code could always work with single-threaded application. But the storage/database implemenation broke that. It forces using multiple threads. But threading is expensive on some platforms or is unwanted by some products. Our patch is single-threaded solution. If other ports or products want use this solution, we can change the name from "Wince" to "SingleThreaded".

(In reply to comment #10) > (In reply to comment #8) > > (From update of attachment 34135 [details] [details]) > > Well, I don't know what this change is doing from the ChangeLog. I assume > > you're trying to make it possible to run Database operations on the main > > thread? > > > > Redirecting to a platform-specific header is generally never the right > > approach: > > #if PLATFORM(WINCE) > > 35 > > 36 #include "wince/DatabaseThreadWince.h" > > 37 > > 38 #else > > > > The abstraction (whatever you're trying to do there) likely belongs at a > > different/higher level. It would be bad, for example if 5 ports all had > > redirects there. > > Platform-independent webkit code could always work with single-threaded > application. But the storage/database implemenation broke that. It forces using > multiple threads. But threading is expensive on some platforms or is unwanted > by some products. > > Our patch is single-threaded solution. If other ports or products want use this > solution, we can change the name from "Wince" to "SingleThreaded". QtWebKit has a build time flag called ENABLE_SINGLE_THREADED to turn off ENABLE_JSC_MULTIPLE_THREADS and WebCore features that create additional threads. I'm interested to make this feature available for QtWebKit (and maybe other ports) as well, not just to the WINCE port. See http://trac.webkit.org/changeset/44411.

This is going to make things quite a bit uglier. Maybe (In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 34135 [details] [details]) > > Well, I don't know what this change is doing from the ChangeLog. I assume > > you're trying to make it possible to run Database operations on the main > > thread? > > > > Redirecting to a platform-specific header is generally never the right > > approach: > > #if PLATFORM(WINCE) > > 35 > > 36 #include "wince/DatabaseThreadWince.h" > > 37 > > 38 #else > > > > The abstraction (whatever you're trying to do there) likely belongs at a > > different/higher level. It would be bad, for example if 5 ports all had > > redirects there. > > Your assumption is correct. We are of the understanding that few if any will > actually want this. If it's desired by others, I agree it should be > refactored. In KDE we had a policy that something doesn't become an API until > at least 2 separate users of it exist, otherwise it's a special case. I think > that's prudent for WebKit too. I don't see the reason to burden the main code > until we have evidence that someone else wants to use it too. Quite honestly, > it's a bit ugly to have a multithreaded and a singlethreaded version in the > same place so I'd rather bury this ugly one away. Adding ifdefs like this really uglies up the code and is not necessary. The abstraction layer is already there: it's StorageAreaSync. The class's API is as follows: void scheduleFinalSync(); void blockUntilImportComplete() const; void scheduleItemForSync(const String& key, const String& value); void scheduleClear(); blockUntilImportComplete could synchronously read in the data. scheduleItemForSync could immediately write an item out. scheduleClear could immediately clear the data. scheduleFinalSync could be a no-op. I believe you can easily/cleanly change things so the StorageSyncManager needn't be instantiated for your port. If you needed ideas on how, I could take a closer look. For WINCE, you could just not compile LocalStorageThread.cpp, LocalStorageTask.cpp, StorageAreaSync.cpp, and StorageAreaManager.cpp and instead provide your own versions (if any) of these files (which would be in a singleThread or wince directory). We use this trick all the time in Chromium (for example, with StorageNamespace.cpp). Let me know if you need any more specific advice, but please don't commit this patch as is.

> Adding ifdefs like this really uglies up the code and is not necessary. The > abstraction layer is already there: it's StorageAreaSync. The class's API is > as follows: "single-threaded" is a little different from "sync". Our single-threaded solution is also "async". It performs one task every time when timer is fired. > > void scheduleFinalSync(); > void blockUntilImportComplete() const; > void scheduleItemForSync(const String& key, const String& value); > void scheduleClear(); > > blockUntilImportComplete could synchronously read in the data. > scheduleItemForSync could immediately write an item out. scheduleClear could > immediately clear the data. scheduleFinalSync could be a no-op. > > I believe you can easily/cleanly change things so the StorageSyncManager > needn't be instantiated for your port. If you needed ideas on how, I could > take a closer look. > > For WINCE, you could just not compile LocalStorageThread.cpp, > LocalStorageTask.cpp, StorageAreaSync.cpp, and StorageAreaManager.cpp and > instead provide your own versions (if any) of these files (which would be in a > singleThread or wince directory). We use this trick all the time in Chromium > (for example, with StorageNamespace.cpp). This is what we're doing right now. but when upstream multi-threaded solution changes, we have to change again. > > Let me know if you need any more specific advice, but please don't commit this > patch as is.

> > Let me know if you need any more specific advice, but please don't commit this > patch as is. We could implement and maintain any file by our own. but the point is trying to share code with other ports without adding pains. Not saying we're going to commit it as it is. but I cannot see why a single-threaded solution shouldn't go in.

Comment on attachment 34135 [details] the patch r- based on comment #14 to get this out of the review queue.

First of all, good point on being sync and single threaded being different. Got mixed up there. (In reply to comment #14) > > > > Let me know if you need any more specific advice, but please don't commit this > > patch as is. > > We could implement and maintain any file by our own. but the point is trying to > share code with other ports without adding pains. Not saying we're going to > commit it as it is. but I cannot see why a single-threaded solution shouldn't > go in. I think a single-threaded solution should go in, but not as a pile of #ifdefs. I'd be OK with having _one_ section of StorageAreaSync and and StorageSyncManager within #ifdef's, but I'm not a reviewer so I'm not sure if even that is typically acceptable. Here are some specific comments: > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index e597233..53be1d0 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,25 @@ > +2009-08-05 Yong Li <yong.li@torchmobile.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Need a short description and bug URL (OOPS!) > + https://bugs.webkit.org/show_bug.cgi?id=28019 > + > + * storage/DatabaseThread.h: > + * storage/LocalStorageThread.h: > + * storage/StorageAreaSync.cpp: > + (WebCore::StorageAreaSync::StorageAreaSync): > + (WebCore::StorageAreaSync::performImport): > + (WebCore::StorageAreaSync::sync): > + (WebCore::StorageAreaSync::performSync): > + * storage/StorageAreaSync.h: > + (WebCore::StorageAreaSync::blockUntilImportComplete): > + (WebCore::StorageAreaSync::markImported): > + * storage/StorageSyncManager.cpp: > + (WebCore::StorageSyncManager::close): > + (WebCore::StorageSyncManager::scheduleImport): > + (WebCore::StorageSyncManager::scheduleSync): > + > 2009-07-21 Yong Li <yong.li@torchmobile.com> > > Reviewed by NOBODY (OOPS!). > diff --git a/WebCore/storage/DatabaseThread.h b/WebCore/storage/DatabaseThread.h > index 5aab5fd..c744b86 100644 > --- a/WebCore/storage/DatabaseThread.h > +++ b/WebCore/storage/DatabaseThread.h > @@ -1,5 +1,6 @@ > /* > * Copyright (C) 2007, 2008 Apple Inc. All rights reserved. > + * Copyright (C) 2009 Torch Mobile, Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > @@ -29,6 +30,13 @@ > #define DatabaseThread_h > > #if ENABLE(DATABASE) > + > +#if PLATFORM(WINCE) > + > +#include "wince/DatabaseThreadWince.h" > + > +#else > + > #include <wtf/Deque.h> > #include <wtf/HashMap.h> > #include <wtf/HashSet.h> > @@ -79,5 +87,7 @@ private: > > } // namespace WebCore > > +#endif // PLATFORM(WINCE) > + > #endif // ENABLE(DATABASE) > #endif // DatabaseThread_h > diff --git a/WebCore/storage/LocalStorageThread.h b/WebCore/storage/LocalStorageThread.h > index 3d58427..45178e8 100644 > --- a/WebCore/storage/LocalStorageThread.h > +++ b/WebCore/storage/LocalStorageThread.h > @@ -1,5 +1,6 @@ > /* > * Copyright (C) 2008 Apple Inc. All Rights Reserved. > + * Copyright (C) 2009 Torch Mobile, Inc. All Rights Reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > @@ -28,6 +29,12 @@ > > #if ENABLE(DOM_STORAGE) > > +#if PLATFORM(WINCE) > + > +#include "wince/LocalStorageThreadWince.h" Is there anything in this file? Maybe the above #if should say ENABLE(DOM_STORAGE) && !PLATFORM(WINCE) ? Or maybe you can (with few or no ifdefs) eliminate any .h file dependencies on this? > + > +#else > + > #include <wtf/HashSet.h> > #include <wtf/MessageQueue.h> > #include <wtf/PassRefPtr.h> > @@ -71,6 +78,8 @@ namespace WebCore { > > } // namespace WebCore > > +#endif // PLATFORM(WINCE) > + > #endif // ENABLE(DOM_STORAGE) > > #endif // LocalStorageThread_h > diff --git a/WebCore/storage/StorageAreaSync.cpp b/WebCore/storage/StorageAreaSync.cpp > index 01d2a65..5182fd6 100644 > --- a/WebCore/storage/StorageAreaSync.cpp > +++ b/WebCore/storage/StorageAreaSync.cpp > @@ -55,8 +55,13 @@ StorageAreaSync::StorageAreaSync(PassRefPtr<StorageSyncManager> storageSyncManag > , m_syncManager(storageSyncManager) > , m_clearItemsWhileSyncing(false) > , m_syncScheduled(false) > +#if !PLATFORM(WINCE) > , m_importComplete(false) > +#endif > { > +#if PLATFORM(WINCE) > + performImport(); > +#else > ASSERT(m_storageArea); > ASSERT(m_syncManager); > > @@ -64,6 +69,7 @@ StorageAreaSync::StorageAreaSync(PassRefPtr<StorageSyncManager> storageSyncManag > // not silently ignoring it. https://bugs.webkit.org/show_bug.cgi?id=25894 > if (!m_syncManager->scheduleImport(this)) > m_importComplete = true; > +#endif Why not just implement your own syncManager and have it work as you'd like it to? > } > > StorageAreaSync::~StorageAreaSync() > @@ -160,7 +166,9 @@ void StorageAreaSync::syncTimerFired(Timer<StorageAreaSync>*) > > void StorageAreaSync::performImport() > { > +#if !PLATFORM(WINCE) > ASSERT(!isMainThread()); > +#endif > ASSERT(!m_database.isOpen()); > > String databaseFilename = m_syncManager->fullDatabaseFilename(m_storageArea->securityOrigin()); > @@ -204,7 +212,9 @@ void StorageAreaSync::performImport() > return; > } > > +#if !PLATFORM(WINCE) > MutexLocker locker(m_importLock); > +#endif > > HashMap<String, String>::iterator it = itemMap.begin(); > HashMap<String, String>::iterator end = itemMap.end(); > @@ -212,12 +222,15 @@ void StorageAreaSync::performImport() > for (; it != end; ++it) > m_storageArea->importItem(it->first, it->second); > > +#if !PLATFORM(WINCE) > // Break the (ref count) cycle. > m_storageArea = 0; > m_importComplete = true; > m_importCondition.signal(); > +#endif > } > > +#if !PLATFORM(WINCE) > void StorageAreaSync::markImported() > { > ASSERT(!isMainThread()); > @@ -250,10 +263,13 @@ void StorageAreaSync::blockUntilImportComplete() const > ASSERT(m_importComplete); > ASSERT(!m_storageArea); > } > +#endif > > void StorageAreaSync::sync(bool clearItems, const HashMap<String, String>& items) > { > +#if !PLATFORM(WINCE) > ASSERT(!isMainThread()); > +#endif > > if (!m_database.isOpen()) > return; > @@ -309,7 +325,9 @@ void StorageAreaSync::sync(bool clearItems, const HashMap<String, String>& items > > void StorageAreaSync::performSync() > { > +#if !PLATFORM(WINCE) > ASSERT(!isMainThread()); > +#endif How about making this a function somewhere. isBackgroundThread. This could be implemented either here or the StorageSyncManager. The implementation for single threaded could just return true. The normal implementation could be !isMainThread(); > > bool clearItems; > HashMap<String, String> items; > diff --git a/WebCore/storage/StorageAreaSync.h b/WebCore/storage/StorageAreaSync.h > index e436bef..fa64984 100644 > --- a/WebCore/storage/StorageAreaSync.h > +++ b/WebCore/storage/StorageAreaSync.h > @@ -46,7 +46,11 @@ namespace WebCore { > ~StorageAreaSync(); > > void scheduleFinalSync(); > +#if PLATFORM(WINCE) > + void blockUntilImportComplete() const {} > +#else > void blockUntilImportComplete() const; > +#endif There's no need to make this inline. > > void scheduleItemForSync(const String& key, const String& value); > void scheduleClear(); > @@ -83,10 +87,14 @@ namespace WebCore { > bool m_clearItemsWhileSyncing; > bool m_syncScheduled; > > +#if PLATFORM(WINCE) > + void markImported() {} > +#else > mutable Mutex m_importLock; > mutable ThreadCondition m_importCondition; > mutable bool m_importComplete; > void markImported(); > +#endif It's a nit, but I don't see why markImported should be inlined here. The mutex and condition could just be ifdef'ed off with no #else. > diff --git a/WebCore/storage/StorageSyncManager.cpp b/WebCore/storage/StorageSyncManager.cpp > index a935242..b90d180 100644 > --- a/WebCore/storage/StorageSyncManager.cpp > +++ b/WebCore/storage/StorageSyncManager.cpp > @@ -72,7 +72,9 @@ String StorageSyncManager::fullDatabaseFilename(SecurityOrigin* origin) > > void StorageSyncManager::close() > { > +#if !PLATFORM(WINCE) > ASSERT(isMainThread()); > +#endif Why are these necessary? Since there's only one thread, wouldn't it always be on the main thread? If it's absolutely necessary, it could be factored into a function like I mentioned above.

(In reply to comment #16) > > Here are some specific comments: Reviewing is appreciated no matter you're reviewer or not. Thanks. > > Is there anything in this file? Maybe the above #if should say > ENABLE(DOM_STORAGE) && !PLATFORM(WINCE) ? Going to change this to ENABLE(SINGLE_THREADED) to make it help QtWebkit > Why not just implement your own syncManager and have it work as you'd like it > to? Create another SyncManager is a cleaner option. But the point is sharing as much code as possible. I'm afraid some reviewers would suggest I share the same file. > > How about making this a function somewhere. isBackgroundThread. This could be > implemented either here or the StorageSyncManager. > > The implementation for single threaded could just return true. The normal > implementation could be !isMainThread(); > > There's no need to make this inline. > > It's a nit, but I don't see why markImported should be inlined here. The mutex > and condition could just be ifdef'ed off with no #else. > > > Why are these necessary? Since there's only one thread, wouldn't it always be > on the main thread? > > If it's absolutely necessary, it could be factored into a function like I > mentioned above. good ideas. Thanks!

(In reply to comment #17) > (In reply to comment #16) > > Why not just implement your own syncManager and have it work as you'd like it > > to? > > Create another SyncManager is a cleaner option. But the point is sharing as > much code as possible. I'm afraid some reviewers would suggest I share the same > file. What about this: """ // License #includes void some function { } void more functions { } #if !USE(SINGLE_THREAD) void multi-threaded function { } void more multi-threaded funcitons { } #endif """ And then implement these in another file within storage/single_thread/blah.cpp (or even in an #else statement below). The biggest thing I'm against here is spaghetti code. If it's between code copying and spaghetti code we should DEFINITELY pick the former. That said, I think 75% of the #ifdefs can be eliminated by a couple tiny refactorings.

Created attachment 35081 [details] refactored 1. change storage/wince to storage/single-threaded 2. use ENABLE(SINGLE_THREADED) so that it can be used by QtWebkit 3. create a separate StorageSyncManager.cpp for single-threaded solution, leaving the old one not touched now, changes on existing files are only for headers.

Looking better! Here's some comments: Ignoring all DB code. (Not sure, but it might be best if you broke it into its own patch/bug.) > diff --git a/WebCore/storage/LocalStorageThread.h b/WebCore/storage/LocalStorageThread.h > index 3d58427..176e279 100644 > --- a/WebCore/storage/LocalStorageThread.h > +++ b/WebCore/storage/LocalStorageThread.h > @@ -1,5 +1,6 @@ > /* > * Copyright (C) 2008 Apple Inc. All Rights Reserved. > + * Copyright (C) 2009 Torch Mobile, Inc. All Rights Reserved. My understanding is that copyright changes should only be made for substantial contributions to a file. (This applies to many of the other files below as well. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > @@ -28,6 +29,12 @@ > > #if ENABLE(DOM_STORAGE) > > +#if ENABLE(SINGLE_THREADED) > + > +#include "single-threaded/LocalStorageThread.h" > + > +#else Instead of this, please do what you did for StorageAreaSync.h. It's much cleaner, IMHO. Sharing .cpp code is nice, but I think sharing .h code is essential. > diff --git a/WebCore/storage/single-threaded/LocalStorageThread.cpp b/WebCore/storage/single-threaded/LocalStorageThread.cpp > new file mode 100644 > index 0000000..8dcb902 > --- /dev/null > +++ b/WebCore/storage/single-threaded/LocalStorageThread.cpp > @@ -0,0 +1,83 @@ > +/* > + * Copyright (C) 2009 Torch Mobile, Inc. All rights reserved. There are still chunks of existing code in here, right? If so, you should probably add Apple's name back to the copyright. > diff --git a/WebCore/storage/single-threaded/StorageAreaSync.cpp b/WebCore/storage/single-threaded/StorageAreaSync.cpp > new file mode 100644 > index 0000000..ac21e7c > --- /dev/null > +++ b/WebCore/storage/single-threaded/StorageAreaSync.cpp There sure is a lot of copied code in here. Why not just put the functions that are completely different in here, #ifdef out the ones you define here in the main file...but do it in _one_ chunk (i.e. move them all to the end so you can do it with one ifdef), and factor out any little differences into their own functions? Also, if there are any files that you copy whole-sale, you probably need to svn cp them. I know you use git and git can't do this, but I think it's worth it to be able to see what's copied and what's not.

> Instead of this, please do what you did for StorageAreaSync.h. It's much > cleaner, IMHO. Sharing .cpp code is nice, but I think sharing .h code is > essential. will think about it > > > diff --git a/WebCore/storage/single-threaded/LocalStorageThread.cpp b/WebCore/storage/single-threaded/LocalStorageThread.cpp > > new file mode 100644 > > index 0000000..8dcb902 > > --- /dev/null > > +++ b/WebCore/storage/single-threaded/LocalStorageThread.cpp > > @@ -0,0 +1,83 @@ > > +/* > > + * Copyright (C) 2009 Torch Mobile, Inc. All rights reserved. > > There are still chunks of existing code in here, right? If so, you should > probably add Apple's name back to the copyright. Which chunk? > > > diff --git a/WebCore/storage/single-threaded/StorageAreaSync.cpp b/WebCore/storage/single-threaded/StorageAreaSync.cpp > > new file mode 100644 > > index 0000000..ac21e7c > > --- /dev/null > > +++ b/WebCore/storage/single-threaded/StorageAreaSync.cpp > > There sure is a lot of copied code in here. Apple's copyright is still remained in this file. Why not just put the functions > that are completely different in here, #ifdef out the ones you define here in > the main file...but do it in _one_ chunk (i.e. move them all to the end so you > can do it with one ifdef), and factor out any little differences into their own > functions? Don't how different multithreaded and single-threaded will be in the future. same consideration for header files...

(In reply to comment #21) > > Instead of this, please do what you did for StorageAreaSync.h. It's much > > cleaner, IMHO. Sharing .cpp code is nice, but I think sharing .h code is > > essential. > > will think about it > > > > > > diff --git a/WebCore/storage/single-threaded/LocalStorageThread.cpp b/WebCore/storage/single-threaded/LocalStorageThread.cpp > > > new file mode 100644 > > > index 0000000..8dcb902 > > > --- /dev/null > > > +++ b/WebCore/storage/single-threaded/LocalStorageThread.cpp > > > @@ -0,0 +1,83 @@ > > > +/* > > > + * Copyright (C) 2009 Torch Mobile, Inc. All rights reserved. > > > > There are still chunks of existing code in here, right? If so, you should > > probably add Apple's name back to the copyright. > > Which chunk? I didn't compare anything specifically, but some of it looked familiar. If not, ignore this comment. > > > > > > diff --git a/WebCore/storage/single-threaded/StorageAreaSync.cpp b/WebCore/storage/single-threaded/StorageAreaSync.cpp > > > new file mode 100644 > > > index 0000000..ac21e7c > > > --- /dev/null > > > +++ b/WebCore/storage/single-threaded/StorageAreaSync.cpp > > > > There sure is a lot of copied code in here. > > Apple's copyright is still remained in this file. My point was that we can minimize the copied code (not that there were copyright issues). > > Why not just put the functions > > that are completely different in here, #ifdef out the ones you define here in > > the main file...but do it in _one_ chunk (i.e. move them all to the end so you > > can do it with one ifdef), and factor out any little differences into their own > > functions? > > Don't how different multithreaded and single-threaded will be in the future. > same consideration for header files... I don't understand this comment. If you're saying that things might diverge more in the future, so you wanted to just split them now, then I'd say that you should code for the way things are now and let future patch authors split it if necessary.

Created attachment 35120 [details] more refactoring 1. remove headers for single-threaded, instead, directly modify on the existing headers 2. single-threaded StorageAreaSync.cpp only contains those functions that are very different with multi-threaded StorageAreaSync.cpp

> This could queue up another task right? Is it ever possible it could go first? No, it's FIFO >Even if not now, maybe you should remove the item before running the task to >avoid regressions in the future? ok. >I think you can ASSERT_NOT_REACHED() here since terminate is what normally >calls this, IIRC. I see this in LocalStorageTask::performTask() case TerminateThread: m_thread->performTerminate(); break; >I'm fine with you factoring out the initialization done in StorageAreaSync's >constructor into another function so that you only need to redefine that. The >only price you'd pay is that m_importComplete would be there again and you >don't use it...but it's only one bool. I'm worried that you're going to break >a lot as people add/remove items from this constructor. >This is a nasty function to fork in order to get rid of only one mutex. I >can't think of any clean way to do it without forking or an #ifdef, though. >If you want to switch it back to an ifdef, I guess it might be better. Your >choice. sigh. k. I'll try to make a new patch. >This is wrong. It should simply explain that the import is done upon >initialization, so this function is a no-op. >On the other hand, maybe you should just do initialization lazily upon the >first call to this? good point.

> >On the other hand, maybe you should just do initialization lazily upon the > >first call to this? > > good point. Hm... I just found that if I do lazy initialization, blockUntilImportComplete() will be broken. That's why I have to performImport in the constructor.

Created attachment 38369 [details] more cleanup share more code of multi-threaded solution

Created attachment 38370 [details] fix a problem in last patch oops, need to add "const" to ImportMutexLocker input

Created attachment 38371 [details] more refactoring sorry for uploading too frequently. hope this can be the last one

Created attachment 38372 [details] onImported => didImport

Almost there (as far as I can tell). Only one major issue. I took a quick look at the DB code as well and it looked good, but I'm not an expert in that area. > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index a2a5dec..211f92a 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,36 @@ > +2009-08-21 Yong Li <yong.li@torchmobile.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Single-threaded Storage/Dababase solution > + This patch also replaces storage/wince > + with storage/single-threaded More detail would be nice. Especially about the single-threaded implementation. > diff --git a/WebCore/storage/StorageAreaSync.cpp b/WebCore/storage/StorageAreaSync.cpp > index 01d2a65..680c1a0 100644 > --- a/WebCore/storage/StorageAreaSync.cpp > +++ b/WebCore/storage/StorageAreaSync.cpp > @@ -332,6 +305,47 @@ void StorageAreaSync::performSync() > enableSuddenTermination(); > } > > +#if !ENABLE(SINGLE_THREADED) > + > +// FIXME: In the future, we should allow use of StorageAreas 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. > +void StorageAreaSync::blockUntilImportComplete() const > +{ > + ASSERT(isMainThread()); > + > + // Fast path to avoid locking. > + if (m_importComplete) > + return; > + > + MutexLocker locker(m_importLock); > + while (!m_importComplete) > + m_importCondition.wait(m_importLock); > + ASSERT(m_importComplete); > + ASSERT(!m_storageArea); > +} > + > +void StorageAreaSync::scheduleImport() > +{ > + m_importComplete = false; > + // FIXME: If it can't import, then the default WebKit behavior should be that of private browsing, > + // not silently ignoring it. https://bugs.webkit.org/show_bug.cgi?id=25894 > + if (!m_syncManager->scheduleImport(this)) > + m_importComplete = true; > +} > + > +void StorageAreaSync::didImport() > +{ > + m_importComplete = true; > + m_importCondition.signal(); Why not move the setting m_storageArea (and its comment) here too? There should be a comment somewhere that this function needs to happen under a lock. It's not obvious to me the difference between "markImported" and "didImport". I can't think of much better, so maybe a 1 line comment above them to the effect of "// Must be called under the m_importLock. Otherwise, call markImported" and "// If you already hold the m_importLock, use didImport". I'm also not wild about the name. Maybe importComplete? I guess I kind of am bike-shedding, though. And yes, all of this is a nit...but I think it's worth cleaning up. > +} > + > +#endif > + > } // namespace WebCore > > #endif // ENABLE(DOM_STORAGE) > diff --git a/WebCore/storage/StorageAreaSync.h b/WebCore/storage/StorageAreaSync.h > index e436bef..94ce59d 100644 > --- a/WebCore/storage/StorageAreaSync.h > +++ b/WebCore/storage/StorageAreaSync.h > @@ -1,5 +1,6 @@ > /* > * Copyright (C) 2008, 2009 Apple Inc. All Rights Reserved. > + * Copyright (C) 2009 Torch Mobile, Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > @@ -78,15 +79,41 @@ namespace WebCore { > void syncTimerFired(Timer<StorageAreaSync>*); > void sync(bool clearItems, const HashMap<String, String>& items); > > - Mutex m_syncLock; > HashMap<String, String> m_itemsPendingSync; > bool m_clearItemsWhileSyncing; > bool m_syncScheduled; > > + void markImported(); > + void scheduleImport(); > + void didImport(); > + > +#if ENABLE(SINGLE_THREADED) > + bool isBackgroundThread() const { return true; } > + struct SyncMutexLocker > + { > + SyncMutexLocker(void*) {} > + }; > + struct ImportMutexLocker > + { > + ImportMutexLocker(const void*) {} > + }; > +#else > + Mutex m_syncLock; > + mutable bool m_importComplete; > mutable Mutex m_importLock; > mutable ThreadCondition m_importCondition; > - mutable bool m_importComplete; > - void markImported(); > + bool isBackgroundThread() const { return !isMainThread(); } > + friend class SyncMutexLocker; > + friend class ImportMutexLocker; > + struct SyncMutexLocker: private MutextLocker > + { > + SyncMutexLocker(StorageAreaSync* storageAreaSync) : MutextLocker(storageAreaSync->m_syncLock) {} > + }; > + struct ImportMutexLocker: private MutextLocker > + { > + ImportMutexLocker(const StorageAreaSync* storageAreaSync) : MutextLocker(storageAreaSync->m_importLock) {} > + }; I'm sorry, but this feels really ugly to me. Why not create a dummy Mutex class (where locks and unlocks are inline no-ops)? Maybe stick it in the WTF? Then all the code is identical, and the only difference is when you define the m_syncLock here. Any halfway sane compiler will inline the no-op and thus it'll be 0 perf penalty. > +#endif > }; > > } // namespace WebCore > diff --git a/WebCore/storage/single-threaded/LocalStorageThread.cpp b/WebCore/storage/single-threaded/LocalStorageThread.cpp > new file mode 100644 > index 0000000..bc6f2ab > --- /dev/null > +++ b/WebCore/storage/single-threaded/LocalStorageThread.cpp > @@ -0,0 +1,90 @@ > +/* > + * Copyright (C) 2009 Torch Mobile, Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * This library is distributed in the hope that i will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Library General Public License for more details. > + * > + * You should have received a copy of the GNU Library General Public License > + * along with this library; see the file COPYING.LIB. If not, write to > + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, > + * Boston, MA 02110-1301, USA. > + */ > + > +#include "config.h" > +#include "LocalStorageThread.h" > + > +#if ENABLE(DOM_STORAGE) > +#if ENABLE(SINGLE_THREADED) > + > +#include "LocalStorageTask.h" > +#include "StorageAreaSync.h" > + > +namespace WebCore { > + > +PassRefPtr<LocalStorageThread> LocalStorageThread::create() > +{ > + return adoptRef(new LocalStorageThread); > +} > + > +LocalStorageThread::LocalStorageThread() > +: m_timer(this, &LocalStorageThread::timerFired) > +{ > +} > + > +bool LocalStorageThread::start() > +{ > + return true; > +} > + > +void LocalStorageThread::timerFired(Timer<LocalStorageThread>*) > +{ > + if (m_queue.isEmpty()) > + return; > + > + RefPtr<LocalStorageTask> task = m_queue.first(); > + m_queue.removeFirst(); > + task->performTask(); > + if (!m_queue.isEmpty()) > + m_timer.startOneShot(0); > +} > + > +void LocalStorageThread::scheduleImport(PassRefPtr<StorageAreaSync> area) > +{ > + m_queue.append(LocalStorageTask::createImport(area)); > + if (!m_timer.isActive()) > + m_timer.startOneShot(0); > +} > + > +void LocalStorageThread::scheduleSync(PassRefPtr<StorageAreaSync> area) > +{ > + m_queue.append(LocalStorageTask::createSync(area)); > + if (!m_timer.isActive()) > + m_timer.startOneShot(0); > +} > + > +void LocalStorageThread::terminate() > +{ > + m_queue.clear(); > + m_timer.stop(); > +} > + > +void LocalStorageThread::performTerminate() > +{ > + m_queue.clear(); > + m_timer.stop(); I said: >I think you can ASSERT_NOT_REACHED() here since terminate is what normally >calls this, IIRC. You said: >I see this in LocalStorageTask::performTask() > case TerminateThread: > m_thread->performTerminate(); > break; I'm pretty sure this task is only ever created by the terminate() call above...which, in your version of this class does NOT schedule such a task. So I think the assert shoudl go in. > +} > + > +} > +#endif // ENABLE(SINGLE_THREADED) > +#endif // ENABLE(DOM_STORAGE) > diff --git a/WebCore/storage/single-threaded/StorageAreaSync.cpp b/WebCore/storage/single-threaded/StorageAreaSync.cpp > new file mode 100644 > index 0000000..6a224f9 > --- /dev/null > +++ b/WebCore/storage/single-threaded/StorageAreaSync.cpp > @@ -0,0 +1,51 @@ > +/* > + * Copyright (C) 2009 Torch Mobile, Inc. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * 2. Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in the > + * documentation and/or other materials provided with the distribution. > + * > + * This library is distributed in the hope that i will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Library General Public License for more details. > + * > + * You should have received a copy of the GNU Library General Public License > + * along with this library; see the file COPYING.LIB. If not, write to > + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, > + * Boston, MA 02110-1301, USA. > + */ > + > +#include "config.h" > +#include "StorageAreaSync.h" > + > +#if ENABLE(DOM_STORAGE) > +#if ENABLE(SINGLE_THREADED) > + > +namespace WebCore { > + > +void StorageAreaSync::scheduleImport() > +{ > + // Do a blocking import because we cannot "blockUntilImportComplete" with one thread nit: Maybe end these sentences with periods? > + performImport(); > +} > + > +void StorageAreaSync::didImport() > +{ > +} > + > +void StorageAreaSync::blockUntilImportComplete() const > +{ > + // We have already imported in scheduleImport() > +} > + > +} // namespace WebCore > + > +#endif // ENABLE(SINGLE_THREADED) > +#endif // ENABLE(DOM_STORAGE) > +

(In reply to comment #30) > > Why not move the setting m_storageArea (and its comment) here too? > thought about that too. > It's not obvious to me the difference between "markImported" and "didImport". > I can't think of much better, so maybe a 1 line comment above them to the > effect of "// Must be called under the m_importLock. Otherwise, call > markImported" and "// If you already hold the m_importLock, use didImport". > > I'm also not wild about the name. Maybe importComplete? I guess I kind of am > bike-shedding, though. > > And yes, all of this is a nit...but I think it's worth cleaning up. > solution: markImportedWithoutLockingMutex() > I'm sorry, but this feels really ugly to me. > > Why not create a dummy Mutex class (where locks and unlocks are inline no-ops)? > Maybe stick it in the WTF? Then all the code is identical, and the only > difference is when you define the m_syncLock here. Any halfway sane compiler > will inline the no-op and thus it'll be 0 perf penalty. good idea. > > I said: > >I think you can ASSERT_NOT_REACHED() here since terminate is what normally > >calls this, IIRC. > > You said: > >I see this in LocalStorageTask::performTask() > > case TerminateThread: > > m_thread->performTerminate(); > > break; > > I'm pretty sure this task is only ever created by the terminate() call > above...which, in your version of this class does NOT schedule such a task. So > I think the assert shoudl go in. you're right

(In reply to comment #31) > (In reply to comment #30) > > > > > Why not move the setting m_storageArea (and its comment) here too? > > > thought about that too. Oops, I thought I removed that comment. I'm fine with it either way, but if you do change it, be sure to add it to the single-threaded version of the function as well. > > > It's not obvious to me the difference between "markImported" and "didImport". > > I can't think of much better, so maybe a 1 line comment above them to the > > effect of "// Must be called under the m_importLock. Otherwise, call > > markImported" and "// If you already hold the m_importLock, use didImport". > > > > I'm also not wild about the name. Maybe importComplete? I guess I kind of am > > bike-shedding, though. > > > > And yes, all of this is a nit...but I think it's worth cleaning up. > > > > solution: markImportedWithoutLockingMutex() Duh...yeah, that is the webkit way. My bad.

Created attachment 38393 [details] more refactoring less modification in StorageAreaSync.cpp

Comment on attachment 38393 [details] more refactoring oops. found a bug

Created attachment 38396 [details] fixed is it perfect now? also removed "mutable" for m_importCompleted, because that's not necessary even in old code.

LGTM > diff --git a/WebCore/storage/StorageAreaSync.h b/WebCore/storage/StorageAreaSync.h > index e436bef..c167864 100644 > --- a/WebCore/storage/StorageAreaSync.h > +++ b/WebCore/storage/StorageAreaSync.h > @@ -1,5 +1,6 @@ > /* > * Copyright (C) 2008, 2009 Apple Inc. All Rights Reserved. > + * Copyright (C) 2009 Torch Mobile, Inc. All rights reserved. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > @@ -78,15 +79,31 @@ namespace WebCore { > void syncTimerFired(Timer<StorageAreaSync>*); > void sync(bool clearItems, const HashMap<String, String>& items); > > - Mutex m_syncLock; > HashMap<String, String> m_itemsPendingSync; > bool m_clearItemsWhileSyncing; > bool m_syncScheduled; > > + void scheduleImport(); > + void markImported(); > + void markImportedWithoutLockingMutex(); > + > +#if ENABLE(SINGLE_THREADED) > + bool isBackgroundThread() const { return true; } > + // Hack WTF::MutexLocker and just do nothing > + enum DummyMutex { > + m_syncLock, > + m_importLock, > + }; > + struct MutexLocker { > + MutexLocker(DummyMutex) {} > + }; Not exactly how I would have done it, but if a real reviewer is fine with it, then I don't care that much. > +#else > + Mutex m_syncLock; > + bool m_importComplete; > mutable Mutex m_importLock; > mutable ThreadCondition m_importCondition; > - mutable bool m_importComplete; > - void markImported(); > + bool isBackgroundThread() const { return !isMainThread(); } > +#endif > }; > > } // namespace WebCore

(In reply to comment #36) > LGTM > > > > > + // Hack WTF::MutexLocker and just do nothing > > + enum DummyMutex { > > + m_syncLock, > > + m_importLock, > > + }; > > + struct MutexLocker { > > + MutexLocker(DummyMutex) {} > > + }; > > Not exactly how I would have done it, but if a real reviewer is fine with it, > then I don't care that much. I have to declare m_syncLock and m_importLock in order to make MutexLocker locker(m_syncLock) compile for single-threaded solution. The best way I can do is use an enum which won't have any overhead.

change the tile

I haven't fully looked thru this, but here are some comments... 62 void DatabaseThread::timerFired(Timer<DatabaseThread>*) 63 { 64 if (m_queue.isEmpty()) 65 return; 66 67 RefPtr<DatabaseTask> task = m_queue.first(); 68 task->performTask(); 69 m_queue.removeFirst(); 70 if (!m_queue.isEmpty()) 71 m_timer.startOneShot(0); 72 } 73 74 void DatabaseThread::scheduleTask(PassRefPtr<DatabaseTask> task) 75 { 76 m_queue.append(task); 77 if (!m_timer.isActive()) 78 m_timer.startOneShot(0); 79 } 80 81 void DatabaseThread::scheduleImmediateTask(PassRefPtr<DatabaseTask> task) 82 { 83 task->performTask(); 84 } As written, this will recursively execute immediate tasks when called during task execution (which can happen). I think thats a little dicey as the caller may depend on execution to be deferred. You could rework this slightly to avoid any potential problems along those lines... here's one way to do that void timerFired() { ASSERT(immedQ.isEmpty()); if (Q.isEmpty()) return; task = Q.first(); Q.removeFirst(); performTask(task); performAllImmediateTasks(); set timer if Q is not empty } void performTask(task) { isPerformingTask = true; task->perform(); isPerformingTask = false; } void scheduleImmediateTask(PassRefPtr<DatabaseTask> task) { immediateQ.prepend(task); // prepend to get the order the multi-threaded system gets i think if (!isPerformingTask ) performAllImmediateTasks(); } void performAllImmediateTasks(PassRefPtr<DatabaseTask> task) { while (!immediateQ.isEmpty()) { task = immediateQ.first(); immediateQ.removeFirst(); // remove before calling perform! performTask(task); } } * WebCore/storage/DatabaseThread.h What is with the diffs here? Its picking most of the file as "changed", but I think the only material changes are around the #if ENABLE(SINGLE_THREADED) mods... is that right?

I think these assertions can be made too... > void performTask(task) { ASSERT(!isPerformingTask); > isPerformingTask = true; > task->perform(); > isPerformingTask = false; > } > > void scheduleImmediateTask(PassRefPtr<DatabaseTask> task) { ASSERT(isPerformingTask || immediateQ.isEmpty()); > immediateQ.prepend(task); > if (!isPerformingTask ) > performAllImmediateTasks(); > }

(In reply to comment #39) > * WebCore/storage/DatabaseThread.h > > What is with the diffs here? Its picking most of the file as "changed", but I > think the only material changes are around the #if ENABLE(SINGLE_THREADED) > mods... is that right? Hm.. I lost the diff for this one. I'll update soon. Re: scheduleImmediateTask The caller of scheduleImmediateTask() generally blocks until the task finishes. With single-threaded solution, we have no choice except performing it immediately. If scheduleImmediateTask() just returns without performing the task, the application could hang up forever.

(In reply to comment #41) > (In reply to comment #39) > > > * WebCore/storage/DatabaseThread.h > > > > What is with the diffs here? Its picking most of the file as "changed", but I > > think the only material changes are around the #if ENABLE(SINGLE_THREADED) > > mods... is that right? > > Hm.. I lost the diff for this one. I'll update soon. > never mind. the patch is OK. Yeah, the changes are all in side #if ENABLE(SINGLE_THREADED). I just added indentation to follow webkit coding guideline

(In reply to comment #41) > (In reply to comment #39) > > > * WebCore/storage/DatabaseThread.h > > > > What is with the diffs here? Its picking most of the file as "changed", but I > > think the only material changes are around the #if ENABLE(SINGLE_THREADED) > > mods... is that right? > > Hm.. I lost the diff for this one. I'll update soon. > > Re: scheduleImmediateTask > > The caller of scheduleImmediateTask() generally blocks until the task finishes. > With single-threaded solution, we have no choice except performing it > immediately. If scheduleImmediateTask() just returns without performing the > task, the application could hang up forever. You're missing a use case. Let me point it out for you... What you say is true of calls to scheduleImmediateTask() made on the main thread, but not true of the calls to scheduleImmediateTask() made on the database thread. There are instances where the database thread schedules tasks back on it self for the purposes of unwinding the stack prior to continuing. The lockAcquired() method is called on the database thread in the multiproces impl while running a DatabaseTask. void SQLTransaction::lockAcquired() { m_lockAcquired = true; m_nextStep = &SQLTransaction::openTransactionAndPreflight; LOG(StorageAPI, "Scheduling openTransactionAndPreflight immediately for transaction %p\n", this); m_database->scheduleTransactionStep(this, true); } Notice that call to scheduleTransactionStep(this, TRUE) above. void Database::scheduleTransactionStep(SQLTransaction* transaction, bool immediately) { if (!m_document->databaseThread()) return; RefPtr<DatabaseTransactionTask> task = DatabaseTransactionTask::create(transaction); LOG(StorageAPI, "Scheduling DatabaseTransactionTask %p for the transaction step\n", task.get()); if (immediately) m_document->databaseThread()->scheduleImmediateTask(task.release()); else m_document->databaseThread()->scheduleTask(task.release()); } In your impl, you've just gone recursive.

r- see previous comment

(In reply to comment #43) > (In reply to comment #41) > > (In reply to comment #39) > > > > void Database::scheduleTransactionStep(SQLTransaction* transaction, bool > immediately) > { > if (!m_document->databaseThread()) > return; > > RefPtr<DatabaseTransactionTask> task = > DatabaseTransactionTask::create(transaction); > LOG(StorageAPI, "Scheduling DatabaseTransactionTask %p for the transaction > step\n", task.get()); > if (immediately) > m_document->databaseThread()->scheduleImmediateTask(task.release()); > else > m_document->databaseThread()->scheduleTask(task.release()); > } > > In your impl, you've just gone recursive. Hm.. this case must be introduced after I implement the single-threaded database. All other cases of using scheduleImmediateTask is like this: task->lockForSynchronousScheduling(); m_document->databaseThread()->scheduleImmediateTask(task); task->waitForSynchronousCompletion(); The solution you provide above will make this hang up forever. Probably we should add an argument to scheduleImmediateTask(task, waitUntilFinish). What do you think?

> Hm.. this case must be introduced after I implement the single-threaded > database. All other cases of using scheduleImmediateTask is like this: > > task->lockForSynchronousScheduling(); > m_document->databaseThread()->scheduleImmediateTask(task); > task->waitForSynchronousCompletion(); > > The solution you provide above will make this hang up forever. Hmmmm... void scheduleImmediateTask(PassRefPtr<DatabaseTask> task) { immediateQ.prepend(task); if (!isPerformingTask ) performAllImmediateTasks(); } That will execute the task prior to return provided we're not nested. Sooo... Where are calls of this nature made... > task->lockForSynchronousScheduling(); > m_document->databaseThread()->scheduleImmediateTask(task); > task->waitForSynchronousCompletion(); If none of these callsites are nested within 'callbacks' from database thread there would't be a deadlock. But that's difficult for me to ASSERT as by thinking it true, especially since the callbacks run script, which call .open(newDatabase), which runs the block of code above. > Probably we should add an argument to scheduleImmediateTask(task, > waitUntilFinish). What do you think? I think we may already have a proxy for such a flag buried in there. How 'bout this... DatabaseTask::isSynchronous() { return m_synchronousMutex.get() ? true : false; } void performTask(task) { bool saveIsPerformingTask = isPerformingTask isPerformingTask = true; task->perform(); isPerformingTask = saveIsPerformingTask; } void scheduleImmediateTask(PassRefPtr<DatabaseTask> task) { if (task->isSynchronous()) performTask(task); else immediateQ.prepend(task); if (!isPerformingTask ) performAllImmediateTasks(); }

Created attachment 39734 [details] fix scheduleImmediateTask() problem Add an argument waitUntilFinish to scheduleImmediateTask() When it's true, multi-threaded solution calls waitSync...., and single-threaded solution just performs the task. The caller is responsible to protect deadlock problems. when it's false, just put the task to the head of the queue.

(In reply to comment #46) > > Hm.. this case must be introduced after I implement the single-threaded > > database. All other cases of using scheduleImmediateTask is like this: > > > > task->lockForSynchronousScheduling(); > > m_document->databaseThread()->scheduleImmediateTask(task); > > task->waitForSynchronousCompletion(); > > > > The solution you provide above will make this hang up forever. > > Hmmmm... > > void scheduleImmediateTask(PassRefPtr<DatabaseTask> task) { > immediateQ.prepend(task); > if (!isPerformingTask ) > performAllImmediateTasks(); > } > > That will execute the task prior to return provided we're not nested. Sooo... > > Where are calls of this nature made... > > > task->lockForSynchronousScheduling(); > > m_document->databaseThread()->scheduleImmediateTask(task); > > task->waitForSynchronousCompletion(); > > If none of these callsites are nested within 'callbacks' from database thread > there would't be a deadlock. But that's difficult for me to ASSERT as by > thinking it true, especially since the callbacks run script, which call > .open(newDatabase), which runs the block of code above. > > > Probably we should add an argument to scheduleImmediateTask(task, > > waitUntilFinish). What do you think? > > I think we may already have a proxy for such a flag buried in there. > > How 'bout this... > > DatabaseTask::isSynchronous() { > return m_synchronousMutex.get() ? true : false; > } > > void performTask(task) { > bool saveIsPerformingTask = isPerformingTask > isPerformingTask = true; > task->perform(); > isPerformingTask = saveIsPerformingTask; > } > > void scheduleImmediateTask(PassRefPtr<DatabaseTask> task) { > if (task->isSynchronous()) > performTask(task); > else > immediateQ.prepend(task); > > if (!isPerformingTask ) > performAllImmediateTasks(); > } Ha, I didn't see your message while implementing it. Check my new patch. I don't use another immediate queue, instead, just put the task to the head of the existing queue.

> Ha, I didn't see your message while implementing it. > > Check my new patch. I don't use another immediate queue, instead, just put the > task to the head of the existing queue. I think what you've got works for me, thnx :) No second Q is definitely a feature. LGTM

Wait... i take that lgtm back... 109 // Make this task the first one in the queue 110 m_queue.prepend(task); 111 if (!m_timer.isActive()) 112 m_timer.startOneShot(0); If we do this while waiting to finish a sync task that needs this immediate 'task' to complete... i think we're hosed.

(In reply to comment #50) > Wait... i take that lgtm back... > > 109 // Make this task the first one in the queue > 110 m_queue.prepend(task); > 111 if (!m_timer.isActive()) > 112 m_timer.startOneShot(0); > > > If we do this while waiting to finish a sync task that needs this immediate > 'task' to complete... i think we're hosed. Hi again, You may be able implement performAllImmediateTasks without a secondQ, just keep track of how many immediate tasks have been prepended. Also I think keeping the 'sync' task callsites looking different than callsites scheduling async tasks may actually be a good thing, the trailing true or false 'wait' argument is a little subtle for this. > DatabaseTask::isSynchronous() { > return m_synchronousMutex.get() ? true : false; > } > > void performTask(task) { > bool saveIsPerformingTask = isPerformingTask > isPerformingTask = true; > task->perform(); > isPerformingTask = saveIsPerformingTask; > } > void scheduleImmediateTask(PassRefPtr<DatabaseTask> task) { if (task->isSynchronous()) performTask(task); else { numImmediate++; Q.prepend(task); } if (!isPerformingTask ) performAllImmediateTasks(); } void performAllImmediateTasks(PassRefPtr<DatabaseTask> task) { ASSERT(numImmediate >= 0); while (numImmediate) { task = Q.first(); Q.removeFirst(); // remove before calling perform! numImmediate--; performTask(task); } }

(In reply to comment #50) > Wait... i take that lgtm back... > > 109 // Make this task the first one in the queue > 110 m_queue.prepend(task); > 111 if (!m_timer.isActive()) > 112 m_timer.startOneShot(0); > > > If we do this while waiting to finish a sync task that needs this immediate > 'task' to complete... i think we're hosed. In that case, waitUntilFinish should true. (In reply to comment #51) 1) I still like an argument for this. It at least can save duplicate code for calling "lockSync..." and "waitSync...". Also, we can even remove those sync methods and objects for #if ENABLE(SINGLE_THREADED)... 2) I don't like performImmediateTasks, because this could block UI. When "waitUntilFinish" is false, "immediate" just means the task should be inserted to the first position of the task queue. So we don't have to perform them all together in one shot. We can still perform one task every time, so that UI won't be blocked by running too many tasks.

(In reply to comment #52) > (In reply to comment #50) > > Wait... i take that lgtm back... > > > > 109 // Make this task the first one in the queue > > 110 m_queue.prepend(task); > > 111 if (!m_timer.isActive()) > > 112 m_timer.startOneShot(0); > > > > > > If we do this while waiting to finish a sync task that needs this immediate > > 'task' to complete... i think we're hosed. > > In that case, waitUntilFinish should true. I think you have to look again. In the usage of scheduleImmediateTask that I pointed out earlier, the 'wait' value will always be false. Now consider what happens if that particular usage is scheduleImmediateTask is invoked in the act of performing Database::openAndVerifyVersion().

(In reply to comment #53) > (In reply to comment #52) > > (In reply to comment #50) > > > Wait... i take that lgtm back... > > > > > > 109 // Make this task the first one in the queue > > > 110 m_queue.prepend(task); > > > 111 if (!m_timer.isActive()) > > > 112 m_timer.startOneShot(0); > > > > > > > > > If we do this while waiting to finish a sync task that needs this immediate > > > 'task' to complete... i think we're hosed. > > > > In that case, waitUntilFinish should true. > > I think you have to look again. > > In the usage of scheduleImmediateTask that I pointed out earlier, the 'wait' > value will always be false. Now consider what happens if that particular usage > is scheduleImmediateTask is invoked in the act of performing > Database::openAndVerifyVersion(). In Database::openAndVerifyVersion(), we have to perform the task immediately for single-threaded version. "wait" must be true here.

> > > > If we do this while waiting to finish a sync task that needs this immediate > > > > 'task' to complete... i think we're hosed. > > > > > > In that case, waitUntilFinish should true. > > > > I think you have to look again. > > > > In the usage of scheduleImmediateTask that I pointed out earlier, the 'wait' > > value will always be false. Now consider what happens if that particular usage > > is scheduleImmediateTask is invoked in the act of performing > > Database::openAndVerifyVersion(). > > In Database::openAndVerifyVersion(), we have to perform the task immediately > for single-threaded version. "wait" must be true here. Nothing in your CL makes 'wait' true in at the callsite i referred to earlier. Furthermore, if it did... it would preform the task recursively which we don't want. Right?

(In reply to comment #55) > > Nothing in your CL makes 'wait' true in at the callsite i referred to earlier. ? I don't understand. I pass "true" to scheduleImmeidateTask() in this case. Check the patch. > Furthermore, if it did... it would preform the task recursively which we don't > want. > > Right? I just know how to depend myself now :) Don't worry about recursive when "wait" is true. See this: 1) In which thread can waitForSynchronousCompletion() be called? I would assume it's definitely the main thread. because the database worker thread cannot wait itself (deadlock!). and there's no reason that other threads call it, right? So, only main thread calls waitForSynchronousCompletion(). 2) in multi-threaded solution: tasks are performed only in database worker thread. right? With 1) and 2), even with multi-threaded solution, you cannot waitForSynchronousCompletion() when performing a task. Otherwise, you get deadlock! Conclusion: waitForSynchronousCompletion() should never be called when performing a task (for both multi & single threaded). So why do I worry about that? :)

First some follow up with you on scheduleImmediate, but then a new topic. (In reply to comment #56) > (In reply to comment #55) > > > > Nothing in your CL makes 'wait' true in at the callsite i referred to earlier. > > ? I don't understand. I pass "true" to scheduleImmeidateTask() in this case. > Check the patch. Sure, but that's not the callsite i'm referring you to. Again, notice the call to m_database->scheduleTransactionStep(this, true) in SQLTransaction::lockAcquired(). That results in a call to scheduleImmediateTask with 'wait' being false. If the processing of any 'sync' tasks involved an additional 'immediate' task like this, the single-threaded impl would get wedged. I just looked thru all of the 'sync' tasks. So as of right now, none of them involve additional immediate tasks. But imo, depending on that remaining true going forward is a little dicey. In particular, it looks like the DatabaseTableNamesTask needs some work. Irrespective of your patch, that particular sync task can wedge the system. I'm not sure what the solution to that will be, it may or may not involve the kind of immediate tasks (schedule from the db thread back onto itself) that will give your single-threaded impl grief. > > Furthermore, if it did... it would preform the task recursively which we don't > > want. > > > > Right? > > I just know how to depend myself now :) Don't worry about recursive when "wait" > is true. See this: > > 1) In which thread can waitForSynchronousCompletion() be called? > > I would assume it's definitely the main thread. because the database worker > thread cannot wait itself (deadlock!). and there's no reason that other threads > call it, right? Yes, only the main thread. > > So, only main thread calls waitForSynchronousCompletion(). > > 2) in multi-threaded solution: tasks are performed only in database worker > thread. right? > > With 1) and 2), even with multi-threaded solution, you cannot > waitForSynchronousCompletion() when performing a task. Otherwise, you get > deadlock! > > Conclusion: waitForSynchronousCompletion() should never be called when > performing a task (for both multi & single threaded). So why do I worry about > that? :) Here's why... There is a difference between 'immediate' and 'synchronious'. The background thread can schedule 'immediate' tasks to be performed. And if and when that happens while performing a 'sync' task on behalf of the main thread, your single-threaded impl is going to be in trouble. As of right now, there are no 'immediate' tasks scheduled while performing any of the 'sync' tasks... but that might change. ***** New topic... The number of DatabaseThread instances in the single-threaded impl. Looks like the single-threaded impl still has one instance per document. I think you probably want to have one instance <period> for your use case. Superficially this makes some sense given in your single-threaded imple DatabaseThread==MainThread, and there's only one of those. But also for a deeper reason... Take a close look at what SQLTransactionCoordinator does. There's logic in there to ensure that two transactions on the same database file (thru different Database instances) don't wedge the system. Each DatabaseThread instance has its own coordinator to keep transactions from butting heads on its particular thread. So... i think you want one instance of DatabaseThread/Coordinator in your world. You're going to have to look more closely at this one. **** Overall... I think your pretty close to a single-threaded solution that won't be too difficult to maintain going forward. Have you run this stuff thru the database layout tests yet?

I'm failing to see the reason why this patch is held up anymore. It seems perfectly reasonable and forcing multithreading has never been done in the past. I'm not sure why storage should mandate it. On platforms that Torch Mobile has worked on this requirement has been unacceptable and we had to remove it with a patch similar to this. As long as it doesn't affect others adversely let's get it checked in and then adjust it as appropriate if other platforms also desire the feature.

(In reply to comment #58) > I'm failing to see the reason why this patch is held up anymore. It seems > perfectly reasonable and forcing multithreading has never been done in the > past. I'm not sure why storage should mandate it. On platforms that Torch > Mobile has worked on this requirement has been unacceptable and we had to > remove it with a patch similar to this. As long as it doesn't affect others > adversely let's get it checked in and then adjust it as appropriate if other > platforms also desire the feature. I didn't mean to "hold it up" or to "force multithreading". Was just trying to help out with a review given my understanding of the Database system. Care to comment specifically on the DatabaseThread/SQLTransactionCoordinator issue i just mentioned... it looks like theres some work to be done on that dimension before you've got something that works when multiple documents (or frames) open connections to the same database file.

> > **** Overall... > I think your pretty close to a single-threaded solution that won't be too > difficult to maintain going forward. Have you run this stuff thru the database > layout tests yet? I've run it though the tests in LayoutTest/storage, and also on real websites. Yes, "sync" is different from "immediate". But in fact, at 4 places in current code, scheduleImmeidateTask() is used as sync with another 2 methods together. (These 4 cases are actually simulating single-threaded thing, and it breaks one of the reasons of using multi-threaded database solution: don't let database failure ruin the main app. In these 4 cases, when database thread crashes, the app will keep hanging up. Also, it blocks UI) My solution is add this argument to distinguish 'sync' and 'immediate'. Another solution is adding another method for 'sync'. I don't need to worry about scheduleImmediateTask(true) being called when performing a task, because if that happens, the multi-threaded solution must also get deadlock. I think you've pointed out a bug for the use of SQLTransactionCoordinator in my code. Thanks a lot. I'll fix that

Oops, I just checked the code. I think multiple instances of DatabaseThread and its coordinator is correct way. Because the coordinator should be able to cancel all pending transactions that belongs to the DatabaseThread/Document

(In reply to comment #61) > Oops, I just checked the code. I think multiple instances of DatabaseThread and > its coordinator is correct way. Because the coordinator should be able to > cancel all pending transactions that belongs to the DatabaseThread/Document Yea... thats why i said "you're going to have to look more closely at this one". I think the single-threaded impl has to reconcile the difference in some way. I doubt any of the existing layout tests will tickle this issue. Since there's an implicit assumption that each Document has its down DBThread, I don't think there are many tests that try to induce deadlocks with multiple Documents involved.

Created attachment 40668 [details] new patch

> Created an attachment (id=40668) [details] > new patch Can you describe what changed since the last patch? Specifically I'm wondering about the thread and transaction coordinator arrangement.

(In reply to comment #64) > > Created an attachment (id=40668) [details] [details] > > new patch > > Can you describe what changed since the last patch? Specifically I'm wondering > about the thread and transaction coordinator arrangement. The changes are made according to Nikolas Zimmermann's suggestions: 1. coding style: remove /t in previous patch 2. replace the code that hacks MutexLocker with a new class StorageMutextLocker. For single-threaded solution, it does nothing.

> The changes are made according to Nikolas Zimmermann's suggestions: > > 1. coding style: remove /t in previous patch > 2. replace the code that hacks MutexLocker with a new class > StorageMutextLocker. For single-threaded solution, it does nothing. I see, if that's the case, I think you've probably got a bug in the single-threaded impl. If i'm right, here's how you should be able to demonstrate the bug. * pageA with a nested iframeB (you need two documents with different DatabaseThreads and accompanying DatabaseCoordinators) * onload, have pageA and iframeB open the same database file (so you have two different Database objects) * start a write transaction in pageA by calling db.transaction(..., false) * in the transaction callback - first call a function in iframeB to start separate write transaction using that nested frames Database object - then call transaction.executeSql(sql) That second transaction started in iframeB will block trying to BEGIN a transaction on a locked database file. After 30 seconds it will time out and fail. Once that timeout expires, the sql statement issued by pageA will execute. I'm relying on tickling the bug by calling into iframeB at just the wrong time, but that's just to make things deterministic. If a second write transaction against the same database file is started while the first is not yet complete, it will block regardless of how that situation arose.

(In reply to comment #66) > > The changes are made according to Nikolas Zimmermann's suggestions: > > > > 1. coding style: remove /t in previous patch > > 2. replace the code that hacks MutexLocker with a new class > > StorageMutextLocker. For single-threaded solution, it does nothing. > > I see, if that's the case, I think you've probably got a bug in the > single-threaded impl. If i'm right, here's how you should be able to > demonstrate the bug. > > * pageA with a nested iframeB (you need two documents with different > DatabaseThreads and accompanying DatabaseCoordinators) > > * onload, have pageA and iframeB open the same database file (so you have two > different Database objects) > > * start a write transaction in pageA by calling db.transaction(..., false) > > * in the transaction callback > - first call a function in iframeB to start separate write transaction > using that nested frames Database object > - then call transaction.executeSql(sql) > > That second transaction started in iframeB will block trying to BEGIN a > transaction on a locked database file. After 30 seconds it will time out and > fail. Once that timeout expires, the sql statement issued by pageA will > execute. > > I'm relying on tickling the bug by calling into iframeB at just the wrong time, > but that's just to make things deterministic. If a second write transaction > against the same database file is started while the first is not yet complete, > it will block regardless of how that situation arose. Hm... This IS a problem. So before running the task when timer fires, I should check if database is locked by another transaction. if yes, I would just delay it and yield. What do you think?

> Hm... This IS a problem. So before running the task when timer fires, I should > check if database is locked by another transaction. if yes, I would just delay > it and yield. What do you think? Ah... i finally got your attention, good ;) You don't want to serialize ALL tasks in that fashion. Only particular tasks operating on 'locked' databases. This is exactly what the transaction coordinator does. For it to do its job, it has to have knowledge of ALL transactions being scheduled on a particular thread. In your case, everything is happening on the 'main' thread. You have some options... a) Have one DatabaseThread (instance) per process instead of one per document. I think this is the most natural arrangment. You may have to poke at the document shutdown logic to detach its particular Databases from the DatabaseThread upon doc going away. b) Have one DatabaseThread (instance) per Document (as it is today), but have them share a TransactionCoordinator, and upon shutdown, unplug that "threads" databases from the coordinator. Either approach seems workable, but option a) makes more sense from the 20000 foot perspective.

(In reply to comment #68) > > Hm... This IS a problem. So before running the task when timer fires, I should > > check if database is locked by another transaction. if yes, I would just delay > > it and yield. What do you think? > > Ah... i finally got your attention, good ;) > > You don't want to serialize ALL tasks in that fashion. Only particular tasks > operating on 'locked' databases. > > This is exactly what the transaction coordinator does. For it to do its job, it > has to have knowledge of ALL transactions being scheduled on a particular > thread. In your case, everything is happening on the 'main' thread. > > You have some options... > > a) Have one DatabaseThread (instance) per process instead of one per document. > I think this is the most natural arrangment. You may have to poke at the > document shutdown logic to detach its particular Databases from the > DatabaseThread upon doc going away. > > b) Have one DatabaseThread (instance) per Document (as it is today), but have > them share a TransactionCoordinator, and upon shutdown, unplug that "threads" > databases from the coordinator. > > Either approach seems workable, but option a) makes more sense from the 20000 > foot perspective. Totally agree. Thanks a lot!

Hi, Michael, I just took a look at the code. I still don't understand the problem very well. It's single-threaded solution, so only one task is performing at one time. Is there a case that a transaction is made up of multiple tasks?

> Is there a case that a transaction is made up of multiple tasks? Yes! Every case. There are many 'steps' to a transaction. Some are performed on the background database thread (sqlite operations), some are performed on the main ui thread (invoking script callbacks to gather statements to be executed and to report results and errors (which may involve additional statements being queued for execution)). Messages (tasks) ping/pongs back and forth between main/database threads until the transaction is complete.

(In reply to comment #71) > > Is there a case that a transaction is made up of multiple tasks? > > Yes! Every case. > > There are many 'steps' to a transaction. Some are performed on the background > database thread (sqlite operations), some are performed on the main ui thread > (invoking script callbacks to gather statements to be executed and to report > results and errors (which may involve additional statements being queued for > execution)). > > Messages (tasks) ping/pongs back and forth between main/database threads until > the transaction is complete. Get it now. But a) has a problem: when the page is closed, how can the DatabaseThread remove all tasks that belongs to the page?

> Get it now. But a) has a problem: when the page is closed, how can the > DatabaseThread remove all tasks that belongs to the page? This is exactly what you need to dig into. You may not have to 'remove all tasks that belong to the page'. If the Database instances for that page are 'closed' in some way such that when tasks execute on their behalf they short circuit. I haven't looked at the details.

Some comments. Btw, I really like how you cleaned up the mutex thing. > diff --git a/WebCore/storage/StorageAreaSync.cpp b/WebCore/storage/StorageAreaSync.cpp > index bec45aa..c8243c0 100644 > --- a/WebCore/storage/StorageAreaSync.cpp > +++ b/WebCore/storage/StorageAreaSync.cpp > @@ -1,5 +1,6 @@ > /* > * Copyright (C) 2008, 2009 Apple Inc. All Rights Reserved. > + * Copyright (C) 2009 Torch Mobile, Inc. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions > @@ -55,16 +56,12 @@ StorageAreaSync::StorageAreaSync(PassRefPtr<StorageSyncManager> storageSyncManag > , m_syncManager(storageSyncManager) > , m_clearItemsWhileSyncing(false) > , m_syncScheduled(false) > - , m_importComplete(false) I really don't like that this is being removed since, but I see why you're doing it. I think we should leave the variable in (it's only 1 byte!) and you should just ignore it in the single threaded code. > void StorageAreaSync::markImported() > { > - ASSERT(!isMainThread()); > - > - MutexLocker locker(m_importLock); > - // Break the (ref count) cycle. > - m_storageArea = 0; > - m_importComplete = true; > - m_importCondition.signal(); > -} > - > -// FIXME: In the future, we should allow use of StorageAreas 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. > -void StorageAreaSync::blockUntilImportComplete() const > -{ > - ASSERT(isMainThread()); > + ASSERT(isBackgroundThread()); > > - // Fast path to avoid locking. > - if (m_importComplete) > - return; > - > - MutexLocker locker(m_importLock); > - while (!m_importComplete) > - m_importCondition.wait(m_importLock); > - ASSERT(m_importComplete); > - ASSERT(!m_storageArea); > + StorageMutexLocker locker(m_importLock); > + markImportedWithoutLockingMutex(); Isn't there only one or two calls to markImported? If so, rename markImportedWithoutLockingMutex to markImported(), have the few callsites to the old version grab the mutex themselves, and add a comment above the function in the header file to state that the caller should hold the import lock when calling. > + > +void StorageAreaSync::scheduleImport() > +{ > + m_importComplete = false; This isn't necessary if you follow my request above. > + // FIXME: If it can't import, then the default WebKit behavior should be that of private browsing, > + // not silently ignoring it. https://bugs.webkit.org/show_bug.cgi?id=25894 > + if (!m_syncManager->scheduleImport(this)) > + m_importComplete = true; > +} > + > +void StorageAreaSync::markImportedWithoutLockingMutex() > +{ Add back in the assert for main thread? > + // Break the (ref count) cycle. > + m_storageArea = 0; > + m_importComplete = true; > + m_importCondition.signal(); > +} > + > +#endif > + > } // namespace WebCore > > #endif // ENABLE(DOM_STORAGE) > diff --git a/WebCore/storage/StorageAreaSync.h b/WebCore/storage/StorageAreaSync.h > index 9d6436f..813a0b8 100644 > --- a/WebCore/storage/StorageAreaSync.h > +++ b/WebCore/storage/StorageAreaSync.h > - mutable Mutex m_importLock; > - mutable ThreadCondition m_importCondition; > - mutable bool m_importComplete; > + void scheduleImport(); > void markImported(); > + void markImportedWithoutLockingMutex(); See my comment towards the end about this... > + > +#if ENABLE(SINGLE_THREADED) > + bool isBackgroundThread() const { return true; } > + struct StorageMutexLocker { nit: might be better to make this a class and then add a public: The main reason is that if someone tries to forward declare this or do anything else somewhat fancy, the difference between struct and class do matter. If you feel strongly, you can leave it though. > + // Do nothing here nit: not sure this comment is very informative. :-) > + StorageMutexLocker(Mutex&) {} nit: isn't a space between the {}'s normal? > + }; > +#else > + typedef MutexLocker StorageMutexLocker; > + bool m_importComplete; > + mutable ThreadCondition m_importCondition; > + bool isBackgroundThread() const { return !isMainThread(); } nit: put above the variables...if nothing else to match how it is in the #if section. > +#endif > + Mutex m_syncLock; > + mutable Mutex m_importLock; > }; > > } // namespace WebCore > diff --git a/WebCore/storage/single-threaded/LocalStorageThread.cpp b/WebCore/storage/single-threaded/LocalStorageThread.cpp > new file mode 100644 > index 0000000..12f11a1 > --- /dev/null > +++ b/WebCore/storage/single-threaded/LocalStorageThread.cpp > + > +PassRefPtr<LocalStorageThread> LocalStorageThread::create() > +{ > + return adoptRef(new LocalStorageThread); > +} > + > +LocalStorageThread::LocalStorageThread() > +: m_timer(this, &LocalStorageThread::timerFired) indent 4 spaces > +{ > +} > +void LocalStorageThread::terminate() > +{ > + m_queue.clear(); > + m_timer.stop(); I believe the multi-threaded code allows the thread to finish processing. If so, then this should probably process all pending events. If you want to exactly match behavior, it would copy the queue and process that so it's impossible for any new items to be added.

Ping? No comments from reviewers in over a month. I don't personally feel qualified to review this. I'm not sure who the database experts are these days...

(In reply to comment #75) > Ping? No comments from reviewers in over a month. I don't personally feel > qualified to review this. I'm not sure who the database experts are these > days... I feel like this is getting fairly close, but there are still some open questions and I know I've made enough changes to LocalStorage that it won't apply cleanly anymore. Is there any chance you could address Michaeln's comments and re-roll another patch? Either way, I think the current one is a clear r-.

Comment on attachment 40668 [details] new patch I agree with Jeremy. Michael's comments are unaddressed. Marking r- officially. That reminds me, jorlow is probably pretty close to needing a reviewer nomination. I should check...

(In reply to comment #76) > (In reply to comment #75) > > Ping? No comments from reviewers in over a month. I don't personally feel > > qualified to review this. I'm not sure who the database experts are these > > days... > > I feel like this is getting fairly close, but there are still some open > questions and I know I've made enough changes to LocalStorage that it won't > apply cleanly anymore. > > Is there any chance you could address Michaeln's comments and re-roll another > patch? > > Either way, I think the current one is a clear r-. I believe this is the plan but it will take some time. Suggest the bug is left open with the patches r-/clear until that happens.