yahoo / squidb

SquiDB is a SQLite database library for Android and iOS
https://github.com/yahoo/squidb/wiki
Apache License 2.0
1.31k stars 132 forks source link

primitives get methods for SquidCursor to prevent autoboxing overhead #118

Closed LouisCAD closed 8 years ago

LouisCAD commented 8 years ago

Hi!

Currently, there's only one SquidCursor#get() implementation, which implies autoboxing for every property you get, which implies an overhead as this video explains. Could you add primitives version of these methods please?

Thanks, and always remember, perfs matters! :)

sbosley commented 8 years ago

Hi @LouisCAD, thanks for opening the issue! We are already aware of the performance implications of autoboxing in SquidCursor, and have made this tradeoff by design. This is partially due to the implementation details of model objects. Model values are stored in ContentValues, to be passed to Android framework methods like SQLiteDatabase#update. ContentValues are just maps under the hood, so primitive values are going to be boxed sooner or later anyways. Primitive values also can't be null, so we designed the API to avoid forcing the user to make a call to cursor.isNull for every field accessed. In our benchmarks and testing, the autoboxing has not been much of an issue in most real-world usage.

That being said, SquidCursor is an instance of CursorWrapper, so all the standard android Cursor methods that work with primitives are still available to you. These are good alternatives if you're not working with model objects and want to squeeze every bit of performance out of the cursor itself. Are you just asking for type safe versions of these methods that work with Property objects rather than column indexes? We can consider that as an API enhancement; it should be very straightforward -- although the caveat about boxing inside model objects would still apply.

LouisCAD commented 8 years ago

Hi @sbosley, first, thanks for replying! I just saw that ContentValues implies autoboxing as it only returns Object versions of primitives, even in getAsInteger(), so Google doesn't listen to Colt McAnlis advices… I think that it doesn't worth it to add primitives methods with this under the hood now, it would add more not that useful code. Thank you, and have a nice day!

sbosley commented 8 years ago

Ok, thanks for the feedback @LouisCAD! It's always possible that someday down the road we'd change the underlying implementation of model objects, so if we ever do we'll definitely revisit this then. Thanks again for opening the discussion!