Switch to more current version of sqlitepcl by CyrusNajmabadi · Pull Request #50841 · dotnet/roslyn

CyrusNajmabadi · 2021-01-27T21:08:40Z

src/Workspaces/Core/Portable/Storage/SQLite/Interop/NativeMethods.cs

CyrusNajmabadi · 2021-01-27T21:10:04Z

src/Workspaces/Core/Portable/Storage/SQLite/Interop/SafeSqliteBlobHandle.cs

 namespace Microsoft.CodeAnalysis.SQLite.Interop
 {
-    internal sealed class SafeSqliteBlobHandle : SafeHandle
+    internal sealed class SafeSqliteBlobHandle : IDisposable


@sharwell was not sure how to handle converting these over. the main issue was that the sqlite core types (like sqlite3_blob) are now SafeHandles themselves, and do not expose the .ptr value.

i thnik we still want these as your leasing pattern effectively means: as long as we create subhandles (i.e. blobs/statements, they should hold the outer sqlite3 instance alive).

CyrusNajmabadi · 2021-01-27T21:10:59Z

src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlStatement.cs

        public static extern int sqlite3_bind_blob(SafeSqliteStatementHandle stmt, int index, byte[] val, int nSize, IntPtr nTransient);

-        internal byte[] GetBlobAt(int columnIndex)
-            => NativeMethods.sqlite3_column_blob(_rawStatement, columnIndex);


CyrusNajmabadi · 2021-01-27T21:26:20Z

src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlStatement.cs

-            => _connection.ThrowIfNotOk(sqlite3_bind_blob(_rawStatement, parameterIndex, value, length, new IntPtr(-1)));
-
-        [DllImport("e_sqlite3.dll", ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)]
-        public static extern int sqlite3_bind_blob(SafeSqliteStatementHandle stmt, int index, byte[] val, int nSize, IntPtr nTransient);


was able to get rid of this ugly hack.

src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage.Accessor.cs

CyrusNajmabadi · 2021-01-28T22:14:27Z

src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs

+        public void WriteTo(Span<byte> span)
+        {
+            var localCopy = _checksum;
+            Contract.ThrowIfFalse(MemoryMarshal.TryWrite(span, ref localCopy));


copy needed, because you can't pass a readonly field by-ref.

CyrusNajmabadi · 2021-01-28T22:22:34Z

@sharwell I've redone how checksums are done entirely in the storage layer. Instead of using generalized paths that go through arbitrary reading/writing of streams, the checksum paths are optimized and can do all their work without an intermediary stream abstraction.

Therzok · 2021-01-30T03:35:15Z

Hey @CyrusNajmabadi, could #23424 be solved as part of this PR?

CyrusNajmabadi · 2021-01-30T04:01:47Z

Yes, quite possibly. I'll look into that next week.

src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlConnection.cs

src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs

Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

CyrusNajmabadi · 2021-02-03T17:57:49Z

@sharwell ?

sharwell · 2021-02-03T21:28:48Z

src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlConnection.cs

-                // can happen when rowId points to a row that hasn't been written to yet.
-                return null;
-            }
+                    Contract.ThrowIfFalse(MemoryMarshal.TryRead(bytes, out Checksum.HashData hashData));


❗ Need to use HashData.FromPointer here.

roslyn/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs

Lines 98 to 102 in 230d0d0

// Avoid a direct dereferencing assignment since sizeof(HashData) may be greater than HashSize.

//

// ex) "https://bugzilla.xamarin.com/show_bug.cgi?id=60298" - LayoutKind.Explicit, Size = 12 ignored with 64bit alignment

// or "https://github.com/dotnet/roslyn/issues/23722" - Checksum throws on Mono 64-bit

return new Checksum(HashData.FromPointer((HashData*)data));

sharwell · 2021-02-03T21:54:15Z

src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs

            => _checksum.WriteTo(writer);

+        public void WriteTo(Span<byte> span)
+            => Contract.ThrowIfFalse(MemoryMarshal.TryWrite(span, ref Unsafe.AsRef(in _checksum)));


This line also suffers from the sizeof(HashData)/HashSize mismatch 😢

src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs

sharwell · 2021-02-03T22:12:30Z

src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs

+            {
+                fixed (byte* bytes = &span.GetPinnableReference())
+                {
+                    *(HashData*)bytes = _checksum;


📝 This should write each field individually rather than trying to copy the entire structure to a new location.

tried that, but i get:

Severity Code Description File Error CS0191 A readonly field cannot be assigned to (except in a constructor or init-only setter of the type in which the field is defined or a variable initializer) C:\github\roslyn\src\Workspaces\Core\Portable\Workspace\Solution\Checksum.cs

sharwell · 2021-02-03T22:13:03Z

src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs

        public void WriteTo(Span<byte> span)
-            => Contract.ThrowIfFalse(MemoryMarshal.TryWrite(span, ref Unsafe.AsRef(in _checksum)));
+        {
+            unsafe


❗ Need to validate span.Length >= HashSize

ghost

Auto-approval

Switch to more current version of sqlitepcl

ec647cc

ghost added the Area-IDE label Jan 27, 2021

CyrusNajmabadi requested a review from sharwell January 27, 2021 21:08

CyrusNajmabadi commented Jan 27, 2021

View reviewed changes

src/Workspaces/Core/Portable/Storage/SQLite/Interop/NativeMethods.cs Show resolved Hide resolved

CyrusNajmabadi commented Jan 27, 2021

View reviewed changes

CyrusNajmabadi added 2 commits January 27, 2021 13:18

Use new APIs

3232654

Add checks

22a18f0

CyrusNajmabadi commented Jan 27, 2021

View reviewed changes

CyrusNajmabadi marked this pull request as ready for review January 27, 2021 23:03

CyrusNajmabadi requested review from a team as code owners January 27, 2021 23:03

sharwell reviewed Jan 28, 2021

View reviewed changes

src/Workspaces/Core/Portable/Storage/SQLite/v2/SQLitePersistentStorage.Accessor.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 11 commits January 28, 2021 11:49

Merge remote-tracking branch 'upstream/master' into newSqlite

ea3cca3

Add specialized no-alloc paths for doing checksum work.

dfe24e0

Share code'

8e232ea

Share more code

a82348f

Assert

d60522b

swap order

5573aec

REmove

701960c

REmove

532d73f

Simplify

8400007

Move abstract method

779a00d

Share code

0f206e8

CyrusNajmabadi commented Jan 28, 2021

View reviewed changes

Revert

88b11f9

Merge remote-tracking branch 'upstream/master' into newSqlite

a64cbdf

CyrusNajmabadi and others added 3 commits January 30, 2021 09:00

Update SqlConnection.cs

3048d19

Increment version

1ef44a0

Fix SQLite safe handle behavior

c97951b

sharwell reviewed Feb 1, 2021

View reviewed changes

src/Workspaces/Core/Portable/Storage/SQLite/v2/Interop/SqlConnection.cs Show resolved Hide resolved

src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs Outdated Show resolved Hide resolved

Update src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs

e6c683b

Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

CyrusNajmabadi requested a review from sharwell February 1, 2021 21:59

add using

432a5bb

sharwell suggested changes Feb 3, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into newSqlite

7fea272

sharwell reviewed Feb 3, 2021

View reviewed changes

Switch to pointers

2af261b

sharwell reviewed Feb 3, 2021

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs Outdated Show resolved Hide resolved

sharwell reviewed Feb 3, 2021

View reviewed changes

Add comment

041f26c

CyrusNajmabadi force-pushed the newSqlite branch from 05427f7 to 041f26c Compare February 3, 2021 22:18

CyrusNajmabadi added 4 commits February 3, 2021 14:32

Simplify

27a58f0

Add check

675cffd

Unsafe

93bacb7

Unify

13dbf49

CyrusNajmabadi requested a review from sharwell February 3, 2021 22:56

sharwell approved these changes Feb 3, 2021

View reviewed changes

CyrusNajmabadi added the auto-merge label Feb 3, 2021

ghost approved these changes Feb 3, 2021

View reviewed changes

ghost merged commit 13a3687 into dotnet:master Feb 4, 2021

ghost added this to the Next milestone Feb 4, 2021

CyrusNajmabadi deleted the newSqlite branch February 4, 2021 01:41

RikkiGibson modified the milestones: Next, 16.10.P1 Feb 17, 2021

This pull request was closed.

	// Avoid a direct dereferencing assignment since sizeof(HashData) may be greater than HashSize.
	//
	// ex) "https://bugzilla.xamarin.com/show_bug.cgi?id=60298" - LayoutKind.Explicit, Size = 12 ignored with 64bit alignment
	// or "https://github.com/dotnet/roslyn/issues/23722" - Checksum throws on Mono 64-bit
	return new Checksum(HashData.FromPointer((HashData*)data));

Conversation

CyrusNajmabadi commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jan 28, 2021

Uh oh!

Therzok commented Jan 30, 2021

Uh oh!

CyrusNajmabadi commented Jan 30, 2021

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Feb 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CyrusNajmabadi commented Jan 27, 2021 •

edited

Loading

CyrusNajmabadi Jan 28, 2021 •

edited

Loading