storesafe / cordova-sqlite-evcore-extbuild-free

Cordova sqlite plugin with Android performance enhancements for PhoneGap Build, GPL v3 or commercial license options
Other
24 stars 13 forks source link

Insertion performance #39

Closed DanielSWolf closed 5 years ago

DanielSWolf commented 6 years ago

In our application, when a user first logs in, a considerable amount of data is transferred to the client and inserted into the database. Depending on the amount of data, the SQL inserts alone take about 10 minutes on a new Android smartphone, which is longer than our customers are willing to accept. So we did some performance analyses.

A typical table row in our application has about 40 columns, but only about 15 of them are typically non-null and none contain BLOBs or long strings. So the pure amount of data shouldn't be an issue.

We insert the data in blocks of 2,000 rows each. Our code for processing such a block of inserts looks roughly like this:

const helper = newPromiseHelper(database);
const tx = helper.newBatchTransaction();
for (const operation of operations) {
  const sql = ...;
  const values = ...;
  tx.executeStatement(sql, values);
}
await tx.commit();

According to our measurements, the last line, which performs the actual commit, takes about 2.3 s on average. That's 2.3 s for 2,000 inserts within a single transaction.

We had a look at the implementation. Right before the jump to native code, SQLitePluginTransaction calls run_batch_flatjson. Each call to run_batch_flatjson takes 600 ms on average. So out of the 2.3 s, only 600 ms are spent in native code. The remaining 1.7 s are spent in the JavaScript part of the SQLite plugin. From the looks of it, the JavaScript code performs a rather elaborate sequence of data transformations, copying all rows from one data structure to the next, then to the next and so on, about five times total.

I know very little about the inner workings of your SQLite plugin, but my gut tells me that it may be possible to significantly reduce these 1.7 s spent repeatedly transforming the data.

What do you think? Is it possible to improve insertion performance?

brodybits commented 6 years ago

Looking through the sql-promise-helper and plugin code involved I can spot the following loops in the JavaScript code:

Note that while there is a for loop in sql-promise-helper (internal executeStatementBatch function) this loop is NOT executed if sql-promise-helper is used with this plugin.

Deeper analysis is coming next.

brodybits commented 6 years ago

I can think of the following opportunities for optimization:

For optimization opportunity 1: The following change (CoffeeScript) to get rid of a for loop in SQLitePlugin.prototype.sqlBatch passes the existing test suite with one minor adjustment needed but with an issue described below:

diff --git a/SQLitePlugin.coffee.md b/SQLitePlugin.coffee.md
index 53dfaf3..b3572a1 100644
--- a/SQLitePlugin.coffee.md
+++ b/SQLitePlugin.coffee.md
@@ -331,29 +331,21 @@

     SQLitePlugin::sqlBatch = (sqlStatements, success, error) ->
       if !sqlStatements || sqlStatements.constructor isnt Array
         throw newSQLError 'sqlBatch expects an array'

-      batchList = []
-
-      for st in sqlStatements
-        if st.constructor is Array
-          if st.length == 0
-            throw newSQLError 'sqlBatch array element of zero (0) length'
-
-          batchList.push
-            sql: st[0]
-            params: if st.length == 0 then [] else st[1]
+      myfn = (tx) ->
+        for st in sqlStatements
+          if st.constructor is Array
+            if st.length == 0
+              throw newSQLError 'sqlBatch array element of zero (0) length'

-        else
-          batchList.push
-            sql: st
-            params: []
+            tx.addStatement st[0],
+              (if st.length == 0 then [] else st[1]), null, null

-      myfn = (tx) ->
-        for elem in batchList
-          tx.addStatement(elem.sql, elem.params, null, null)
+          else
+            tx.addStatement st, [], null, null

       @addTransaction new SQLitePluginTransaction(this, myfn, error, success, true, false)
       return

 ## SQLite plugin transaction object for batching:

However the change above would NOT pass the following test:

        it(suiteName + 'sql batch with changing SQL element', function(done) {
          var db = openDatabase('sql-batch-with-changing-sql.db');

          var mybatch = [
            'DROP TABLE IF EXISTS MyTable',
            'CREATE TABLE MyTable (data)',
            "INSERT INTO MyTable VALUES ('Alice')"
          ];

          db.sqlBatch(mybatch, function() {
            db.executeSql('SELECT * FROM MyTable', [], function (rs) {
              expect(rs.rows.length).toBe(1);
              expect(rs.rows.item(0).data).toBe('Alice');
              db.close(done, done);
            });
          }, function(error) {
            // NOT EXPECTED:
            expect(false).toBe(true);
            expect(error.message).toBe('--');
            db.close(done, done);
          });
          mybatch[2] = "INSERT INTO MyTable VALUES ('Betty')";
        }, MYTIMEOUT);

The following change (CoffeeScript) also passes the new test above (though with the same minor test adjustment still needed):

diff --git a/SQLitePlugin.coffee.md b/SQLitePlugin.coffee.md
index 53dfaf3..fb97dd2 100644
--- a/SQLitePlugin.coffee.md
+++ b/SQLitePlugin.coffee.md
@@ -331,29 +331,23 @@

     SQLitePlugin::sqlBatch = (sqlStatements, success, error) ->
       if !sqlStatements || sqlStatements.constructor isnt Array
         throw newSQLError 'sqlBatch expects an array'

-      batchList = []
+      myStatements = sqlStatements.slice(0)

-      for st in sqlStatements
-        if st.constructor is Array
-          if st.length == 0
-            throw newSQLError 'sqlBatch array element of zero (0) length'
-
-          batchList.push
-            sql: st[0]
-            params: if st.length == 0 then [] else st[1]
+      myfn = (tx) ->
+        for st in myStatements
+          if st.constructor is Array
+            if st.length == 0
+              throw newSQLError 'sqlBatch array element of zero (0) length'

-        else
-          batchList.push
-            sql: st
-            params: []
+            tx.addStatement st[0],
+              (if st.length == 0 then [] else st[1]), null, null

-      myfn = (tx) ->
-        for elem in batchList
-          tx.addStatement(elem.sql, elem.params, null, null)
+          else
+            tx.addStatement st, [], null, null

       @addTransaction new SQLitePluginTransaction(this, myfn, error, success, true, false)
       return

 ## SQLite plugin transaction object for batching:

Marking this sqlBatch optimization opportunity for possible inclusion in June 2018 release of cordova-sqlite-storage (litehelpers/Cordova-sqlite-storage#773).

I will take a look at optimization opportunity 2 another day. I think it will need the same flat JSON enhancement to be applied for iOS/macOS and Windows in addition to Android. This may be targeted for evplus and evmax plugin versions. @DanielSWolf I will contact you privately with options for moving forward.

P.S. My second iteration of optimization 1 would still have some kind of an internal loop in the Array.slice() call but that loop would be run at the native level. My theory is that the internal loop at the native level would be much faster than at the JavaScript level.

brodybits commented 5 years ago

Now solved in litehelpers / cordova-plugin-sqlite-evplus-ext-common-free, available under GPL v3 or evplus commercial license (not covered by commercial license for evcore). Contact for commercial licenses: sales@litehelpers.net