Reviewed by Alice
authorbeidson <beidson@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 Aug 2006 20:10:00 +0000 (20:10 +0000)
committerbeidson <beidson@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 24 Aug 2006 20:10:00 +0000 (20:10 +0000)
        <rdar://problem/4697973> - Unacceptable delay on startup
        <rdar://problem/4690949> - Need to correctly prune unretained pageurls and icons on startup

        This patch was started by me and finished by Mark Rowe - we now special case all retains during
        startup into one huge sql transaction.  Also we track PageURL retains instead of IconURLs so pruning works right.
        Testing with reasonable sets of bookmarks/history (3000), startup time is neglibile.  Testing with a huge set of
        bookmarks (40,000), startup has a noticable delay, but reasonable, and is inline with shipping safari which also
        has a noticeable delay.

        * loader/icon/IconDatabase.cpp:
        (WebCore::IconDatabase::IconDatabase):
        (WebCore::IconDatabase::open): adding an initialStartupTransaction and pageRetainStatement
        (WebCore::IconDatabase::close): do cleanup on the initialStartupSQL stuff
        (WebCore::IconDatabase::retainIconForPageURL): Track initial PageURL retains in the temporary table
        (WebCore::IconDatabase::releaseIconForPageURL): Ditto
        (WebCore::IconDatabase::retainIconURL): We no longer special case this on startup
        (WebCore::IconDatabase::releaseIconURL): We no longer special case this on startup
        (WebCore::IconDatabase::pruneUnretainedIconsOnStartup): Handle the big transaction correctly and quickly
        (WebCore::IconDatabase::syncDatabase): Change the timing log message
        * loader/icon/IconDatabase.h:
        * loader/icon/SQLStatement.cpp:
        (WebCore::SQLStatement::bindText16): Added this - for reusing commonly used statements by just rebinding parameters.
        * loader/icon/SQLStatement.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@16020 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/loader/icon/IconDatabase.cpp
WebCore/loader/icon/IconDatabase.h
WebCore/loader/icon/SQLStatement.cpp
WebCore/loader/icon/SQLStatement.h

index c04be11b7c63dd393e7bfaf11b993de1239878fa..bed98fc70c62b71caa263c7b868e3a30a2f61b0f 100644 (file)
@@ -1,3 +1,31 @@
+2006-08-24  Brady Eidson  <beidson@apple.com>
+
+        Reviewed by Alice
+
+        <rdar://problem/4697973> - Unacceptable delay on startup
+        <rdar://problem/4690949> - Need to correctly prune unretained pageurls and icons on startup
+
+        This patch was started by me and finished by Mark Rowe - we now special case all retains during
+        startup into one huge sql transaction.  Also we track PageURL retains instead of IconURLs so pruning works right.
+        Testing with reasonable sets of bookmarks/history (3000), startup time is neglibile.  Testing with a huge set of 
+        bookmarks (40,000), startup has a noticable delay, but reasonable, and is inline with shipping safari which also 
+        has a noticeable delay.
+
+        * loader/icon/IconDatabase.cpp:
+        (WebCore::IconDatabase::IconDatabase):
+        (WebCore::IconDatabase::open): adding an initialStartupTransaction and pageRetainStatement
+        (WebCore::IconDatabase::close): do cleanup on the initialStartupSQL stuff
+        (WebCore::IconDatabase::retainIconForPageURL): Track initial PageURL retains in the temporary table
+        (WebCore::IconDatabase::releaseIconForPageURL): Ditto
+        (WebCore::IconDatabase::retainIconURL): We no longer special case this on startup
+        (WebCore::IconDatabase::releaseIconURL): We no longer special case this on startup
+        (WebCore::IconDatabase::pruneUnretainedIconsOnStartup): Handle the big transaction correctly and quickly
+        (WebCore::IconDatabase::syncDatabase): Change the timing log message
+        * loader/icon/IconDatabase.h:
+        * loader/icon/SQLStatement.cpp:
+        (WebCore::SQLStatement::bindText16): Added this - for reusing commonly used statements by just rebinding parameters.
+        * loader/icon/SQLStatement.h:
+
 2006-08-24  Nikolas Zimmermann  <zimmermann@kde.org>
 
         Reviewed/landed by Adam.
index f3522375d79747285f913d44daa974607fb4eb86..9ee58dc0763880cfb3e07094a7a88cb4b3f64e52 100644 (file)
@@ -35,6 +35,7 @@
 #include <sys/types.h>
 #include <time.h>
 #include "SQLStatement.h"
+#include "SQLTransaction.h"
 
 
 // FIXME - One optimization to be made when this is no longer in flux is to make query construction smarter - that is queries that are created from
@@ -98,6 +99,8 @@ IconDatabase::IconDatabase()
     , m_startupTimer(this, &IconDatabase::pruneUnretainedIconsOnStartup)
     , m_updateTimer(this, &IconDatabase::updateDatabase)
     , m_initialPruningComplete(false)
+    , m_initialPruningTransaction(0)
+    , m_preparedPageRetainInsertStatement(0)
 {
     
 }
@@ -129,15 +132,18 @@ bool IconDatabase::open(const String& databasePath)
         createDatabaseTables(m_mainDB);
     }
 
+    m_initialPruningTransaction = new SQLTransaction(m_mainDB);
     // We're going to track an icon's retain count in a temp table in memory so we can cross reference it to to the on disk tables
     bool result;
-    result = m_mainDB.executeCommand("CREATE TEMP TABLE IconRetain (url TEXT);");
+    result = m_mainDB.executeCommand("CREATE TEMP TABLE PageRetain (url TEXT);");
     // Creating an in-memory temp table should never, ever, ever fail
     ASSERT(result);
-    
+
     // These are actually two different SQLite config options - not my fault they are named confusingly  ;)
-    m_mainDB.setSynchronous(SQLDatabase::SyncOff);    
+    m_mainDB.setSynchronous(SQLDatabase::SyncOff);
     m_mainDB.setFullsync(false);
+
+    m_initialPruningTransaction->begin();
     
     // Open the in-memory table for private browsing
     if (!m_privateBrowsingDB.open(":memory:"))
@@ -155,6 +161,10 @@ bool IconDatabase::open(const String& databasePath)
 
 void IconDatabase::close()
 {
+    delete m_initialPruningTransaction;
+    if (m_preparedPageRetainInsertStatement)
+        m_preparedPageRetainInsertStatement->finalize();
+    delete m_preparedPageRetainInsertStatement;
     syncDatabase();
     m_mainDB.close();
     m_privateBrowsingDB.close();
@@ -341,27 +351,42 @@ void IconDatabase::retainIconForPageURL(const String& pageURL)
 {
     if (pageURL.isEmpty())
         return;
-        
-    int retainCount;
     
     // If we don't have the retain count for this page, we need to setup records of its retain
     // Otherwise, get the count and increment it
-    if (!m_pageURLToRetainCount.contains(pageURL)) {
+    int retainCount;
+    if (!(retainCount = m_pageURLToRetainCount.get(pageURL))) {
+        m_pageURLToRetainCount.set(pageURL, 1);   
+
+        // If we haven't done initial pruning, we store this retain record in the temporary in-memory table
+        // Note we only keep the URL in the temporary table, not the full retain count, because for pruning-considerations
+        // we only care *if* a pageURL is retained - not the full count.  This call to retainIconForPageURL incremented the PageURL's
+        // retain count from 0 to 1 therefore we may store it in the temporary table
+        // Also, if we haven't done pruning yet, we want to avoid any pageURL->iconURL lookups and the pageURLsPendingDeletion is moot, 
+        // so we bail here and skip those steps
+        if (!m_initialPruningComplete) {
+            String escapedPageURL = escapeSQLString(pageURL);
+            if (!m_preparedPageRetainInsertStatement) {
+                m_preparedPageRetainInsertStatement = new SQLStatement(m_mainDB, "INSERT INTO PageRetain VALUES (?);");
+                m_preparedPageRetainInsertStatement->prepare();
+            }
+            m_preparedPageRetainInsertStatement->reset();
+            m_preparedPageRetainInsertStatement->bindText16(0, pageURL);
+            if (m_preparedPageRetainInsertStatement->step() != SQLITE_DONE)
+                LOG_ERROR("Failed to record icon retention in temporary table for IconURL %s", pageURL.ascii().data());
+            return;
+        }
+        
         // If this pageURL is marked for deletion, bring it back from the brink
         m_pageURLsPendingDeletion.remove(pageURL);
         
-        // The new retain count will be 1
-        retainCount = 1;
-        
-         // If we have an iconURL for this pageURL, we'll now retain the iconURL
+        // If we have an iconURL for this pageURL, we'll now retain the iconURL
         String iconURL = iconURLForPageURL(pageURL);
         if (!iconURL.isEmpty())
             retainIconURL(iconURL);
 
     } else
-        retainCount = m_pageURLToRetainCount.get(pageURL) + 1;
-        
-    m_pageURLToRetainCount.set(pageURL, retainCount);        
+        m_pageURLToRetainCount.set(pageURL, retainCount + 1);   
 }
 
 void IconDatabase::releaseIconForPageURL(const String& pageURL)
@@ -390,9 +415,23 @@ void IconDatabase::releaseIconForPageURL(const String& pageURL)
     // Otherwise, remove all record of the retain count
     m_pageURLToRetainCount.remove(pageURL);   
     
+    // If we haven't done initial pruning, we remove this retain record from the temporary in-memory table
+    // Note we only keep the URL in the temporary table, not the full retain count, because for pruning-considerations
+    // we only care *if* a pageURL is retained - not the full count.  This call to releaseIconForPageURL decremented the PageURL's
+    // retain count from 1 to 0 therefore we may remove it from the temporary table
+    // Also, if we haven't done pruning yet, we want to avoid any pageURL->iconURL lookups and the pageURLsPendingDeletion is moot, 
+    // so we bail here and skip those steps
+    if (!m_initialPruningComplete) {
+        String escapedPageURL = escapeSQLString(pageURL);
+        if (!m_mainDB.executeCommand("DELETE FROM PageRetain WHERE url='" + escapedPageURL + "';"))
+            LOG_ERROR("Failed to delete record of icon retention from temporary table for IconURL %s", pageURL.ascii().data());
+        return;
+    }
+
+    
     // Then mark this pageURL for deletion
     m_pageURLsPendingDeletion.add(pageURL);
-            
+    
     // Grab the iconURL and release it
     String iconURL = iconURLForPageURL(pageURL);
     if (!iconURL.isEmpty())
@@ -410,16 +449,6 @@ void IconDatabase::retainIconURL(const String& iconURL)
         m_iconURLToRetainCount.set(iconURL, 1);
         if (m_iconURLsPendingDeletion.contains(iconURL))
             m_iconURLsPendingDeletion.remove(iconURL);
-            
-        // If we haven't done initial pruning, we store this retain record in the temporary in-memory table
-        // Note we only keep the URL in the temporary table, not the full retain count, because for pruning-considerations
-        // we only care *if* an icon is retained - not the full count.  This call to retainIconURL incremented the IconURL's
-        // retain count from 0 to 1 therefore we may store it in the temporary table
-        if (!m_initialPruningComplete) {
-            String escapedIconURL = escapeSQLString(iconURL);
-            if (!m_mainDB.executeCommand("INSERT INTO IconRetain VALUES ('" + escapedIconURL + "');"))
-                LOG_ERROR("Failed to record icon retention in temporary table for IconURL %s", iconURL.ascii().data());
-        }
     }   
 }
 
@@ -448,16 +477,6 @@ void IconDatabase::releaseIconURL(const String& iconURL)
     
     // And since we delay the actual deletion of icons, so lets add it to that queue
     m_iconURLsPendingDeletion.add(iconURL);
-    
-    // If we haven't done initial pruning, we remove this retain record from the temporary in-memory table
-    // Note we only keep the URL in the temporary table, not the full retain count, because for pruning-considerations
-    // we only care *if* an icon is retained - not the full count.  This call to retainIconURL decremented the IconURL's
-    // retain count from 1 to 0 therefore we may remove it from the temporary table
-    if (!m_initialPruningComplete) {
-        String escapedIconURL = escapeSQLString(iconURL);
-        if (!m_mainDB.executeCommand("DELETE FROM IconRetain WHERE url='" + escapedIconURL + "';"))
-            LOG_ERROR("Failed to delete record of icon retention from temporary table for IconURL %s", iconURL.ascii().data());
-    }
 }
 
 void IconDatabase::forgetPageURL(const String& pageURL)
@@ -633,23 +652,49 @@ void IconDatabase::pruneUnretainedIconsOnStartup(Timer<IconDatabase>*)
     // rdar://4690949 - Need to prune unretained iconURLs here, then prune out all pageURLs that reference
     // nonexistent icons
     
-    // First wipe all IconURLs that aren't retained
-    if (!m_mainDB.executeCommand("DELETE FROM Icon WHERE Icon.URL NOT IN (SELECT * FROM IconRetain);"))
-        LOG_ERROR("Failed to execute SQL to prune unretained icons from the on-disk tables");
-    // Then wipe all PageURLs whose IconURLs are now missing
-    if (!m_mainDB.executeCommand("DELETE FROM PageURL WHERE NOT EXISTS (SELECT * FROM Icon WHERE PageURL.iconID = Icon.iconID);"))
-        LOG_ERROR("Failed to prune dangling PageURLs from the DB");
-    // Finally wipe the temporary IconRetain table
-    if (!m_mainDB.executeCommand("DROP TABLE IconRetain;"))
-        LOG_ERROR("Failed to remove temporary IconRetain table");
-        
+    // First wipe all PageURLs and Icons that aren't retained
+    if (!m_mainDB.executeCommand("DELETE FROM PageURL WHERE PageURL.url NOT IN (SELECT url FROM PageRetain);") ||
+        !m_mainDB.executeCommand("DELETE FROM Icon WHERE Icon.iconID NOT IN (SELECT iconID FROM PageURL);") ||
+        !m_mainDB.executeCommand("DROP TABLE PageRetain;")) {
+        LOG_ERROR("Failed to execute SQL to prune unretained pages and icons from the on-disk tables");
+        m_initialPruningTransaction->rollback();
+    } else
+        m_initialPruningTransaction->commit();
+    delete m_initialPruningTransaction;
+    m_initialPruningTransaction = 0;
+    
+    // Since we lazily retained the pageURLs without getting the iconURLs or retaining the iconURLs, 
+    // we need to do that now
+    // We now should be in a spiffy situation where we know every single pageURL left in the DB is retained, so we are interested
+    // in the iconURLs for all remaining pageURLs
+    // So we can simply add all the remaining mappings, and retain each pageURL's icon once
+    
+    SQLStatement sql(m_mainDB, "SELECT PageURL.url, Icon.url FROM PageURL INNER JOIN Icon ON PageURL.iconID=Icon.iconID");
+    sql.prepare();
+    int result;
+    while((result = sql.step()) == SQLITE_ROW) {
+        String iconURL = sql.getColumnText16(1);
+        m_pageURLToIconURLMap.set(sql.getColumnText16(0), iconURL);
+        retainIconURL(iconURL);
+        LOG(IconDatabase, "Found a PageURL that mapped to %s", iconURL.ascii().data());
+    }
+    if (result != SQLITE_DONE)
+        LOG_ERROR("Error reading PageURL->IconURL mappings from on-disk DB");
+    sql.finalize();
+
+    if (m_preparedPageRetainInsertStatement) {
+        m_preparedPageRetainInsertStatement->finalize();
+        delete m_preparedPageRetainInsertStatement;
+        m_preparedPageRetainInsertStatement = 0;
+    }
     m_initialPruningComplete = true;
     
 #ifndef NDEBUG
     CFTimeInterval duration = CFAbsoluteTimeGetCurrent() - start;
-    LOG(IconDatabase, "Pruning unretained icons took %.4f seconds", duration);
-    if (duration > 1.0) 
-        LOG_ERROR("Pruning unretained icons took %.4f seconds - this is much too long!", duration);
+    if (duration <= 1.0)
+        LOG(IconDatabase, "Pruning unretained icons took %.4f seconds", duration);
+    else
+        LOG(IconDatabase, "Pruning unretained icons took %.4f seconds - this is much too long!", duration);
 #endif
 }
 
@@ -707,9 +752,10 @@ void IconDatabase::syncDatabase()
     
 #ifndef NDEBUG
     CFTimeInterval duration = CFAbsoluteTimeGetCurrent() - start;
-    LOG(IconDatabase, "Updating the database took %.4f seconds", duration);
-    if (duration > 1.0) 
-        LOG_ERROR("Updating the database took %.4f seconds - this is much too long!", duration);
+    if (duration <= 1.0)
+        LOG(IconDatabase, "Updating the database took %.4f seconds", duration);
+    else 
+        LOG(IconDatabase, "Updating the database took %.4f seconds - this is much too long!", duration);
 #endif
 }
 
index 655ec4ac363aa772d8ffc2169bbe8243d03a8460..04cf092319630114f621da03ede661b78d92a4a7 100644 (file)
@@ -51,6 +51,8 @@ namespace WebCore {
 
 class Image;
 class IconDataCache;
+class SQLTransaction;
+class SQLStatement;
 
 class IconDatabase
 {
@@ -166,6 +168,8 @@ private:
     Timer<IconDatabase> m_updateTimer;
     
     bool m_initialPruningComplete;
+    SQLTransaction* m_initialPruningTransaction;
+    SQLStatement* m_preparedPageRetainInsertStatement;
     
     HashMap<String, IconDataCache*> m_iconURLToIconDataCacheMap;
     HashSet<IconDataCache*> m_iconDataCachesPendingUpdate;
index 39d1b5f55f90ad3e8099f913ab5aebf2b4e9c32b..a16cf08e548b231b931711234d585f1e821a1901 100644 (file)
@@ -133,6 +133,16 @@ int SQLStatement::bindText(int index, const char* text, bool copy)
     return lastError();
 }
 
+int SQLStatement::bindText16(int index, const String& text, bool copy)
+{
+    if (copy)
+        sqlite3_bind_text16(m_statement, index, text.characters(), sizeof(UChar) * text.length(), SQLITE_TRANSIENT);
+    else
+        sqlite3_bind_text16(m_statement, index, text.characters(), sizeof(UChar) * text.length(), SQLITE_STATIC);
+    return lastError();
+}
+
+
 int SQLStatement::bindInt64(int index, int64_t integer)
 {
     return sqlite3_bind_int64(m_statement, index, integer);
index 012dbec4d157d0cfea5236c816ee3351b0ddc8e4..09295c3a246a33d6af6d6109e6cbeb68569c82c8 100644 (file)
@@ -45,6 +45,7 @@ public:
     bool isPrepared() { return m_statement; }
     int bindBlob(int index, const void* blob, int size, bool copy = true);
     int bindText(int index, const char* text, bool copy = true);
+    int bindText16(int index, const String& text, bool copy = true);
     int bindInt64(int index, int64_t integer);
 
     int step();