Efcore: Microsoft.Data.Sqlite: Make GetBytes, GetChars, and GetTextReader more efficient

[bricelam]

SqliteDataReader.GetBytes(), GetChars(), and GetTextReader() should leverage SqliteBlob. We should also update GetStream() to cache the rowid ordinal and value as part of this change. (Don't forget to invalidate them in Read and NextResult.)

[bricelam]

Also look into using sqlite3_blob_reopen if it gets added to SQLitePCL.raw https://github.com/ericsink/SQLitePCL.raw/issues/240

[bricelam]

Note, SQLitePCL.raw assumes strings are always encoded using UTF-8. We can too.

[MhdTlb]

I would like to work on this task. any guide PLZ

[bricelam]

Work is described above:

• Update SqliteDataReader.GetBytes() to use GetStream()
• Update GetChars() to use GetStream(); Text is encoded as UTF-8
• Update GetTextReader() to use GetStream(). Again, UTF-8
• Update GetStream() to cache rowidOrdinal

[nmichels]

Hi @bricelam
I'm trying to implement the changes above, but I'm not sure if I'm doing that in the most efficient way, it looks like I'm doing a lot of copies/casts that may not be necessary. Here it goes what I have so far:

[bricelam]

The MemoryStreams look unnecessary. Also, use StreamReader instead of Encoding.GetString(). Feel free to send a PR. We can iterate on the implementation there.

[nmichels]

Hi @bricelam
As you have suggested, I've sent a PR, also removed the memory streams and fixed some regressions that I've caught with the previous changes. The last two steps are missing: Update GetTextReader() - I didn't find this method - to use GetStream(). Again, UTF-8; And Update GetStream() to cache rowidOrdinal.

[nmichels]

Hi @bricelam
It looks like caching is already implemented at GetStream

if (rowidOrdinal < 0)
{
    return new MemoryStream(GetCachedBlob(ordinal), false);
}

Unless we should apply something similar at this line:
return new SqliteBlob(_connection, blobTableName, blobColumnName, rowid, readOnly: true);

Also _blob_cache is being cleared at Read method:
Array.Clear(_blobCache, 0, _blobCache.Length);

And the whole reader is disposed at SqliteDataReader.cs:NextResult

if (_record != null)
{
    _record.Dispose();
    _record = null;
}

Is the method sqlite3_blob_reopen related to the caching strategy? It is already implemented at SQLitePCL.raw

[smitpatel]

I reverted the fix for this since it is causing intermittent seg fault on Linux in Microsoft.Data.Sqlite.Tests.

[bricelam]

I’ll dig into these. May have exposed an existing issue with SqliteBlob.

[bricelam]

AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at SQLitePCL.SQLite3Provider_dynamic_cdecl.SQLitePCL.ISQLite3Provider.sqlite3_blob_close(IntPtr)
   at SQLitePCL.raw.internal_sqlite3_blob_close(IntPtr)
   at SQLitePCL.sqlite3_blob.ReleaseHandle()
   at System.Runtime.InteropServices.SafeHandle.InternalRelease(Boolean)
   at System.Runtime.InteropServices.SafeHandle.Dispose(Boolean)
   at System.Runtime.InteropServices.SafeHandle.Finalize()

[bricelam]

We're probably leaking sqlite_blob objects and the SQLite GC ends up freeing them before the .NET GC. 💥