Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Insertion performance #39

Closed
DanielSWolf opened this issue May 22, 2018 · 3 comments
Closed

Insertion performance #39

DanielSWolf opened this issue May 22, 2018 · 3 comments

Comments

@DanielSWolf
Copy link

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?

@brodycj
Copy link
Contributor

brodycj commented May 22, 2018

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

  • in SQLitePlugin.prototype.sqlBatch ("SQLitePlugin::sqlBatch"):
    • for loop to push all statements & parameters into temporary batchList variable
    • for loop in temporary myfn function that calls "internal" SQLitePluginTransaction.addStatement (SQLitePluginTransaction.prototype.addStatement) function to add each statement to the transaction the same way as this would be done in a "standard" Web SQL transaction
  • in SQLitePluginTransaction.prototype.addStatement:
    • loop through parameters of each statement
  • in SQLitePluginTransaction.prototype.run_batch_flatjson:
    • loop through all SQL statements & parameters to push all into flat JSON list
    • in mycb result callback function: loop through batch results to trigger success & error callback functions

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.

@brodycj
Copy link
Contributor

brodycj commented May 22, 2018

I can think of the following opportunities for optimization:

  • 1: get rid of extra for loop in SQLitePlugin.prototype.sqlBatch
  • 2: fix SQLitePluginTransaction.prototype.addStatement to directly populate the flat JSON list (and per-statement callback list)

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

@brodycj
Copy link
Contributor

brodycj commented Aug 20, 2018

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

@brodycj brodycj closed this as completed Aug 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants