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

Performance Optimization: Fast Column Setters #902

Merged
merged 3 commits into from
Dec 6, 2020

Conversation

cherron-aptera
Copy link
Contributor

@cherron-aptera cherron-aptera commented Dec 18, 2019

This PR changes the way that we do column lookups to be once-per-query rather than once-per row. By doing this, we can speed up large query retrieval by ~500%.

We have to do some funky stuff re: creating dynamic strongly-typed setter delegates, but other libraries have already gone this way (such as Protobuf), so we can follow in their footsteps a bit.

Detailed explanation: Simple Syntax vs. Fast Speed

SQLite.net is very easy to use, and has great facility for mapping from SQLite tables into object properties. However, for very large queries, response times can be a bit on the slow side:

// *** Method 1 ***
var orders = conn.Query<OrderDetail>("select * from [OrderDetail]");

If OrderDetail is 1 million records or so, then performance is very poor:

Retrieved 1000000 records in 14852 ms

Why does it take 15 seconds to query from a local database? This should be FAST!

Traditionally this can be overcome by doing some low-level work and stepping through a SQLite query on your own:

// *** Method 2 ***
var stmt = SQLite3.Prepare2(conn.Handle, "select * from [OrderDetail]");
var orders = new List<OrderDetail>();

while (SQLite3.Step(stmt) == SQLite3.Result.Row)
{
    var id = SQLite3.ColumnInt(stmt, 0);
    var orderId = SQLite3.ColumnInt(stmt, 1);
    var productId = SQLite3.ColumnInt(stmt, 2);
    var quantity = SQLite3.ColumnInt(stmt, 3);
    var name = SQLite3.ColumnString(stmt, 4);

    var orderDetail = new OrderDetail()
    {
        Id = id,
        OrderId = orderId,
        ProductId = productId,
        Quantity = quantity,
        Name = name
    };
    orders.Add(orderDetail);
}

uuuugh. It's much more cumbersome syntax, but the performance boost is undeniable:
Retrieved 1000000 records in 1350 ms.

That's more like it! It runs in a tenth of the time! Both are using SQLite.net with the same query on the same database -- so why is Method 1 so slow?

In-Loop Reflection as the Root of Evil

The core of the problem lies in the fact that the inside of SQLite.net's ExecuteDeferredQuery loop makes a call to PropertyInfo.SetValue (). This is a rather heavy-weight call that uses Reflection every time it's called to make sure that the generic object being passed into it is compatible with the type held by that particular PropertyInfo.

So how do we fix this? Is there a way to have the syntactic sugar of Method 1 with the speed of Method 2?

The good news is yes -- there are two alternatives that people have used to approach this problem.

Fast-Member

The fastest method is to emit dynamic IL and link into that at runtime. This is the method used by the excellent Fast-Member library.

However, there are .NET platforms (such as Xamarin.iOS and Unity IL2CPP) that don't support such shenanigans, so I'd rather not go down that road.

Strongly-Typed Delegates

The second method isn't quite as fast as dynamic IL, but it's still reasonably fast. It involves doing all of the type-checking reflection outside of the query loop, and creating strongly-typed delegates to set property values.

Jon Skeet wrote a blog post that explains this technique, as well as a Stack Overflow answer that summarizes nicely. This is the same technique used to add this same speed-boost to Google's Protobuf library.

I'm not doing everything quite the same way as he is (to be honest, I had a hard time following all of it), but I think the version I created is hopefully easy enough to read, and similarly zippy at runtime.

Note that we avoid quite a bit of confusion by simply skipping the Enum case, and falling back to the original method of simply calling Column.Set() on every row. For enumerated types, then my pull request will simply fall back on the old (and slow) method of calling PropertyInfo.SetValue() on every row. Yes, it's slow -- but at least it's not going to be any slower than it was before this change (and maybe someone else can help figure out the black-magic voodoo to make strongly-typed delegates for enums function).

Measuring Performance

So what does this all boil down to? Well, let's check the performance.

Prior to my pull request:

Method 1: Retrieved 1000000 records in 15009 ms.
Method 2: Retrieved 1000000 records in 1419 ms.

After FastColumnSet:

Method 1: Retrieved 1000000 records in 3087 ms.
Method 2: Retrieved 1000000 records in 1426 ms.

Still not nearly as good as the hand-created mappings, but 15 seconds down to 3 seconds is still an impressive boost!

There's a lot of junk I needed to put in there to make Nullable types work properly (ugh). If I skip Nullable checks and defer nullable types back to the legacy method, then it makes things a tad bit simpler, but I don't think the speed increase is that significant -- it shaves off maybe 500ms or so.

Feedback?

What do you all think? Is this clean enough / general-purpose enough to make it into the main trunk?

I'm certainly open to input on how to make this all cleaner -- it was a bear getting this all to work properly, but I'm very very thankful for a comprehensive unit test suite in SQLite-net. Kudos for that. :)

@cherron-aptera
Copy link
Contributor Author

Note: If anyone else would like to try reproducing my results, the program I used to time performance is here:

https://gist.github.com/cherron-aptera/a31f1de0fd489ffd74db248b62e5a6e5

@@ -2855,20 +2855,29 @@ public IEnumerable<T> ExecuteDeferredQuery<T> (TableMapping map)
var stmt = Prepare ();
try {
var cols = new TableMapping.Column[SQLite3.ColumnCount (stmt)];
var fastColumnSetters = new Action<T, Sqlite3Statement, int>[SQLite3.ColumnCount (stmt)];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we cache a list of fast column setters at the beginning of the loop.

var colType = SQLite3.ColumnType (stmt, i);
var val = ReadCol (stmt, i, colType, cols[i].ColumnType);
cols[i].SetValue (obj, val);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a FastColumnSetter is ever not available for this column, then we don't worry about it and we just fall back to the (slow) PropertyInfo.SetValue() method.

}

if (isNullable) {
var setProperty = (Action<ObjectType, ColumnMemberType?>)Delegate.CreateDelegate (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's frustrating to me how much of this code is almost identically copy-pasted between CreateNullableTypedSetterDelegate and CreateTypedSetterDelegate, but I couldn't find a clean way around it. I don't know if / how ProtoBuf deals with this, but I wonder if they don't support nullable types the way SQLite-net does.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I like your code, my biggest hesitation is how much dupe code the reader layers are accumulating. I think that cleanup work will be for another time though.

…om being called on every row of every query. This greatly improves large query performance because it frontloads the heavy Reflection checks to only be called once per query.
@cherron-aptera
Copy link
Contributor Author

Rebased and re-uploaded the branch to trigger the automatic build checks again.

This is a significant performance optimization, and I would still love to see this get merged into master. I know there's a lot here and it's not super-easy to follow -- is there anything I can do to help to make the review process smoother, @praeclarum ?

@cherron-aptera cherron-aptera changed the title Fast Column Setters Performance Optimization: Fast Column Setters Aug 12, 2020
Copy link
Owner

@praeclarum praeclarum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this. I've been meaning to implement Strongly-Typed Delegates forever now. It's certainly faster on iOS.

I really appreciate all your effort and research into this! 🤠

}

if (isNullable) {
var setProperty = (Action<ObjectType, ColumnMemberType?>)Delegate.CreateDelegate (
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I like your code, my biggest hesitation is how much dupe code the reader layers are accumulating. I think that cleanup work will be for another time though.

@praeclarum praeclarum merged commit 4d95ef3 into praeclarum:master Dec 6, 2020
@tranb3r
Copy link

tranb3r commented May 6, 2021

Seems to cause trouble with SqliteNetExtensions
But it's far beyond my understanding, whether this is an issue in this PR or in SqliteNetExtensions.
@cherron-aptera Any idea ?

[Error] Exception catched !!!System.ArgumentException: method arguments are incompatible
  at System.Delegate.CreateDelegate (System.Type type, System.Object firstArgument, System.Reflection.MethodInfo method, System.Boolean throwOnBindFailure, System.Boolean allowClosed) [0x002e3] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/mcs/class/corlib/System/Delegate.cs:281 
  at System.Delegate.CreateDelegate (System.Type type, System.Object firstArgument, System.Reflection.MethodInfo method) [0x00000] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/mcs/class/corlib/System/Delegate.cs:296 
  at SQLite.FastColumnSetter.CreateNullableTypedSetterDelegate[ObjectType,ColumnMemberType] (SQLite.TableMapping+Column column, System.Func`3[T1,T2,TResult] getColumnValue) [0x00077] in <a9edccd8a27f41fdaae16acd8ec0ca8e>:0 
  at SQLite.FastColumnSetter.GetFastSetter[T] (SQLite.SQLiteConnection conn, SQLite.TableMapping+Column column) [0x000a0] in <a9edccd8a27f41fdaae16acd8ec0ca8e>:0 
  at SQLite.SQLiteCommand+<ExecuteDeferredQuery>d__12`1[T].MoveNext () [0x000d1] in <a9edccd8a27f41fdaae16acd8ec0ca8e>:0 
  at System.Collections.Generic.List`1[T].AddEnumerable (System.Collections.Generic.IEnumerable`1[T] enumerable) [0x00059] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/external/corefx/src/Common/src/CoreLib/System/Collections/Generic/List.cs:1108 
  at System.Collections.Generic.List`1[T]..ctor (System.Collections.Generic.IEnumerable`1[T] collection) [0x00062] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/external/corefx/src/Common/src/CoreLib/System/Collections/Generic/List.cs:87 
  at System.Linq.Enumerable.ToList[TSource] (System.Collections.Generic.IEnumerable`1[T] source) [0x0000e] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/external/corefx/src/System.Linq/src/System/Linq/ToCollection.cs:30 
  at SQLite.SQLiteCommand.ExecuteQuery[T] (SQLite.TableMapping map) [0x00007] in <a9edccd8a27f41fdaae16acd8ec0ca8e>:0 
  at SQLite.SQLiteConnection.Query (SQLite.TableMapping map, System.String query, System.Object[] args) [0x00008] in <a9edccd8a27f41fdaae16acd8ec0ca8e>:0 
  at SQLiteNetExtensions.Extensions.ReadOperations.GetManyToOneChildren[T] (SQLite.SQLiteConnection conn, System.Collections.Generic.IList`1[T] elements, System.Reflection.PropertyInfo relationshipProperty, System.Boolean recursive, System.Collections.Generic.Dictionary`2[TKey,TValue] objectCache) [0x00159] in D:\Dev\Zitch\sqlite-net-extensions\SQLiteNetExtensions\Extensions\ReadOperations.cs:358 
  at SQLiteNetExtensions.Extensions.ReadOperations.GetChildRecursive (SQLite.SQLiteConnection conn, System.Object element, System.Reflection.PropertyInfo relationshipProperty, System.Boolean recursive, System.Collections.Generic.Dictionary`2[TKey,TValue] objectCache) [0x00044] in D:\Dev\Zitch\sqlite-net-extensions\SQLiteNetExtensions\Extensions\ReadOperations.cs:190 
  at SQLiteNetExtensions.Extensions.ReadOperations.GetChildrenRecursive (SQLite.SQLiteConnection conn, System.Object element, System.Boolean onlyCascadeChildren, System.Boolean recursive, System.Collections.Generic.Dictionary`2[TKey,TValue] objectCache) [0x0003a] in D:\Dev\Zitch\sqlite-net-extensions\SQLiteNetExtensions\Extensions\ReadOperations.cs:169 
  at SQLiteNetExtensions.Extensions.ReadOperations.GetChildren[T] (SQLite.SQLiteConnection conn, T element, System.Boolean recursive) [0x00000] in D:\Dev\Zitch\sqlite-net-extensions\SQLiteNetExtensions\Extensions\ReadOperations.cs:110 
  at SQLiteNetExtensionsAsync.Extensions.ReadOperations+<>c__DisplayClass3_0`1[T].<GetChildrenAsync>b__0 () [0x00029] in D:\Dev\Zitch\sqlite-net-extensions\SQLiteNetExtensionsAsync\Extensions\ReadOperations.cs:123 
  at System.Threading.Tasks.Task.InnerInvoke () [0x0000f] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/external/corert/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs:2476 
  at System.Threading.Tasks.Task.Execute () [0x00000] in /Users/builder/jenkins/workspace/archive-mono/2020-02/android/release/external/corert/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs:2319 

@cherron-aptera
Copy link
Contributor Author

@tranb3r It looks like it was attempting to persist a List<> with SqliteNetExtensions -- what type of object was stored in the list, do you know?

If you could possibly set a breakpoint and see what the expected method arguments are, I could look into why this is happening.

It looks like you're doing this on Android? I'm curious to know if it happens on other platforms as well. If we could build an isolated test project, that would help me with reproducing -- but I don't know how much trouble that would be.

@tranb3r
Copy link

tranb3r commented May 6, 2021

A bit more context:

  • yes this is on Android. I don't run this app on other platforms yet.
  • I'm using sqlcipher but it probably does not make a difference
  • there is no list involved. I'm getting recursively (with sqlitenetextensions) object A -> object B -> object C. Both relationships are ManyToOne.

I can definitely set a breakpoint, but could you please tell me where exactly (I guess when calling the CreateDelegate in CreateTypedSetterDelegate method ?) and what info you need ?

I can also try to build a repro project, but I guess it's only relevant to do it on android since the exception is in mono ?

@cherron-aptera
Copy link
Contributor Author

Hmm, good question. Maybe the first step is to see if we can reproduce it in a desktop environment?

@cherron-aptera
Copy link
Contributor Author

@tranb3r If you are able to give me a code snippet for how you're using sqlitenetextensions, I can see about building a sample application from my end?

@tranb3r
Copy link

tranb3r commented May 6, 2021

I've created a simple repro project, with 2 applications:

  • TestSqliteNet.Android : xamarin forms android app
  • TestSqliteNet.Net : netcoreapp3.1 console app

With sqlite-net-pcl 1.8.0-beta, there is an exception on both environments:

  • android : System.ArgumentException: method arguments are incompatible
  • netcoreapp3.1 : System.ArgumentException: Cannot bind to the target method because its signature is not compatible with that of the delegate type

No error with 1.7.335.

The code for the test is in TestClass.cs in the shared lib (TestSqliteNet).
It uses sqlite-net-pcl and SQLiteNetExtensions ; no need for sqlcipher nor async api.

@cherron-aptera Let me know if you need more information.
Thanks!

TestSqliteNet.zip

@cherron-aptera
Copy link
Contributor Author

I haven't yet taken time to try to fix this yet, but wanted to confirm that your test project is excellent -- I am able to run it locally and it fails for me with the same exception that you reported. Well done on this -- thank you!

@tranb3r
Copy link

tranb3r commented May 27, 2021

@cherron-aptera Did you have the time to take a look ? Maybe I should open a new issue ?

@tranb3r
Copy link

tranb3r commented Jun 8, 2021

@cherron-aptera Sorry to insist: should I open a new issue ? Maybe somebody else will have the time to take a look. Thanks.

@cherron-aptera
Copy link
Contributor Author

@tranb3r I'm sorry, I haven't taken time to take a look yet. I will try to do so, but feel free to open an issue. If anyone else has time to look at it also, that would be very welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants