[SQLite] Bump and use byte[] variants added to blob_open for performance by Therzok · Pull Request #25047 · dotnet/roslyn

Therzok · 2018-02-25T21:45:41Z

NOTE: Currently using CI as a build bot, roslyn builds take too much time on my machine.

Ask Mode template not completed

Customer scenario

What does the customer do to get into this situation, and why do we think this
is common enough to address for this release. (Granted, sometimes this will be
obvious "Open project, VS crashes" but in general, I need to understand how
common a scenario is)

Bugs this fixes

(either VSO or GitHub links)

Workarounds, if any

Also, why we think they are insufficient for RC vs. RC2, RC3, or RTW

Risk

This is generally a measure our how central the affected code is to adjacent
scenarios and thus how likely your fix is to destabilize a broader area of code

Performance impact

(with a brief justification for that assessment (e.g. "Low perf impact because no extra allocations/no complexity changes" vs. "Low")

Is this a regression from a previous update?

Root cause analysis

How did we miss it? What tests are we adding to guard against it in the future?

How was the bug found?

(E.g. customer reported it vs. ad hoc testing)

Test documentation updated?

If this is a new non-compiler feature or a significant improvement to an existing feature, update https://github.com/dotnet/roslyn/wiki/Manual-Testing once you know which release it is targeting.

Therzok · 2018-02-25T21:46:59Z

I went with the route of adding another property (<Name>Ptr) simply due to the fact that string versions may also be used in other places.

CyrusNajmabadi · 2018-02-25T21:48:01Z

src/Workspaces/Core/Desktop/Workspace/SQLite/Interop/SqlConnection.cs

there is Util.ToUtf8 and Util.to_utf8?

No space before (

CyrusNajmabadi · 2018-02-25T21:48:45Z

src/Workspaces/Core/Desktop/Workspace/SQLite/Interop/SqlConnection.cs

does this overload need to exist? who is calling into ReadBlob with a string for the table name or column name?

Well, it was public API, so not sure if it's safe to completely remove it (although the class is internal).

I can remove this overload if we don't need it (it's currently not used anywhere)

Yes. Please remove the overload. This type is completely internal to roslyn. We do not have to worry about breaking changes here.

CyrusNajmabadi · 2018-02-25T21:50:22Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorage.Accessor.cs

shouldn't the ColumnName be a byte[] too?

Yep, good catches so far. I haven't compiled locally, so this probably has issues even at compile time :p

Therzok · 2018-02-25T22:07:29Z

src/Workspaces/Core/Desktop/Workspace/SQLite/Interop/Util.cs

@CyrusNajmabadi had forgotten to commit this file.

CyrusNajmabadi · 2018-02-25T22:13:16Z

src/Workspaces/Core/Desktop/Workspace/SQLite/Interop/Util.cs

this should be documented why you're doing this (i presume sqlite requires 0 terminated utf8?).

This was shamelessly copy pasted from sqlitepcl.raw :p

Just to be clear. By 'documented', i meant: 'comment in the code' :)

Yes, I did add some comments.

CyrusNajmabadi · 2018-02-25T22:59:14Z

src/Workspaces/Core/Desktop/Workspace/SQLite/Interop/Util.cs

needs copyright.

sharwell · 2018-02-25T23:13:24Z

src/Workspaces/Core/Desktop/Workspace/SQLite/Interop/Util.cs

💭 This method could be reduced to the following if you want it simple. You may end up with a couple extra NUL characters in the converted string, but it would be usable.

return Encoding.UTF8.GetBytes(text + '\0');

Encoding.UTF8.GetBytes(string) internally calls the byte[] variant, https://github.com/dotnet/corefx/blob/master/src/Common/src/CoreLib/System/Text/Encoding.cs#L770-L781

So, that would incur:
1 string allocation '\0' -> "0"
1 string allocation for the string concat (text + "0")

I used this method for performance.

I used this method for performance.

Relative impact of costs needs to be considered. So, for example, your change helps replace an unending amount of string->byte[] allocations with a single fixed number of allocations (<10). Improving allocation scenarios doesn't mean removing all allocations, just the costly ones :)

that said, i'm fine with the code as is. It's nice and clear and doesn't do anything that seems unnecessary. Seems like a good solution to me.

Ah, I just realized that I changed this locally before I moved the things to happen in the static ctors. I can simplify the code.

jasonmalinowski

Just some nitpicks around naming and stuff. Would like them fixed, but otherwise good.

My standard "are you sure you want to target master?" applies.

jasonmalinowski · 2018-02-26T19:19:33Z

src/Workspaces/Core/Desktop/Workspace/SQLite/Interop/Util.cs

This should probably be called "ToUtf8WithNullTerminator" or something. Seeing initial use my thought was "why do we need a helper?"

jasonmalinowski · 2018-02-26T19:20:00Z

src/Workspaces/Core/Desktop/Workspace/SQLite/Interop/SqlConnection.cs

readonly? Yeah, the array is still mutable but it's a small protection.

Sure thing!

jasonmalinowski · 2018-02-26T19:20:26Z

src/Workspaces/Core/Desktop/Workspace/SQLite/SQLitePersistentStorage.cs

readonly everything here.

jasonmalinowski · 2018-02-26T19:31:28Z

Also, @sharwell or @jinujoe this will probably require a bump in the open source tool to track the new dependency. Who wants to own following up on that?

heejaechang · 2018-02-26T23:05:24Z

nice.

Therzok · 2018-02-27T08:08:35Z

Should be ok now.

Therzok · 2018-02-27T08:59:54Z

I think 15-7 is what we want.

Therzok · 2018-03-01T12:19:19Z

Fixed the Windows VisualStudio projects. Build should go green soon.

Opened another issue about other projects not using the Packages.props values. #25148

Therzok · 2018-03-01T12:57:20Z

@jasonmalinowski any idea what I might be doing wrong with the nuget package version?

jasonmalinowski · 2018-03-01T18:42:38Z

It looks like the package changed from having assets under the win7-x86 RID it's now just win-x86. You'll have to fix those paths. If you crack the nuget package you can see what's inside.

Fixes dotnet#23424

Therzok · 2018-05-22T20:02:52Z

Failures look unrelated?

heejaechang · 2018-05-22T20:07:00Z

@dotnet/roslyn-infrastructure integration test failure? is it known issue?

https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_debug_vs-integration_prtest/1548/testReport/junit/Roslyn.VisualStudio.IntegrationTests.CSharp/CSharpReplIdeFeatures/AddUsing/

jasonmalinowski · 2018-05-22T20:21:41Z

Yes, the queues are currently under maintinence. Given we don't have great unit test coverage of this, I'm tempted to say let's wait until those come back up before merging this.

Have we also done the OSS paperwork? That needs to be done before merging.

Therzok · 2018-05-22T20:59:55Z

On another note, I should probably upgrade to a newer sqlitepclraw, as 1.1.11 is out.

heejaechang · 2018-05-22T21:06:33Z

@jinujoseph no idea on when we are supposed to move the version up. and what paper work we need to do.

jinujoseph · 2018-05-23T02:01:54Z

new version bump will need paper work and understanding of why we would want to move .

jasonmalinowski · 2018-05-23T17:58:50Z

And I should comment: the paperwork is sometimes just rubber-stamped by a tool, so it's possibly quite easy.

Therzok · 2018-05-23T20:33:11Z

Fwiw, I saw submissions for 1.1.11 in osstool

jasonmalinowski · 2018-05-24T20:30:01Z

@Therzok Cool, then it might get rubber stamped. @jinujoseph who is doing the paperwork then, at least to see if it gets the rubber stamp?

jinujoseph · 2018-05-25T17:37:01Z

@ivanbasov is helping with the paperwork here..thanks

ivanbasov · 2018-05-25T20:53:49Z

I've sent a request for the OSS permission approval for sqlite. I will keep you informed.

ivanbasov · 2018-05-29T19:25:15Z

All the paperwork has been approved. We can proceed with the merge. Thank you!

jasonmalinowski · 2018-05-29T19:28:11Z

@jinujoseph This just needs an delegated M2 approval then.

jinujoseph · 2018-05-29T19:41:12Z

Approved to merge for 15.8.Preview3

Therzok requested review from a team as code owners February 25, 2018 21:45

CyrusNajmabadi reviewed Feb 25, 2018

View reviewed changes

Therzok force-pushed the sqlite-blob branch from 22eadcf to 6efbd72 Compare February 25, 2018 21:48

CyrusNajmabadi reviewed Feb 25, 2018

View reviewed changes

Therzok force-pushed the sqlite-blob branch from 6efbd72 to 5449565 Compare February 25, 2018 21:49

CyrusNajmabadi reviewed Feb 25, 2018

View reviewed changes

Therzok force-pushed the sqlite-blob branch 3 times, most recently from 8812e66 to fdf4f0c Compare February 25, 2018 22:06

Therzok commented Feb 25, 2018

View reviewed changes

CyrusNajmabadi reviewed Feb 25, 2018

View reviewed changes

src/Workspaces/Core/Desktop/Workspace/SQLite/Interop/Util.cs Outdated

Copy link
Copy Markdown

Contributor

CyrusNajmabadi Feb 25, 2018

Choose a reason for hiding this comment

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

needs copyright.

Therzok force-pushed the sqlite-blob branch from d34b08d to 038e653 Compare February 25, 2018 23:04

sharwell reviewed Feb 25, 2018

View reviewed changes

Therzok force-pushed the sqlite-blob branch 2 times, most recently from a7bf633 to 5f95e8a Compare February 26, 2018 07:46

jasonmalinowski reviewed Feb 26, 2018

View reviewed changes

jasonmalinowski approved these changes Feb 26, 2018

View reviewed changes

Therzok changed the base branch from master to dev15.7.x February 27, 2018 09:00

Therzok force-pushed the sqlite-blob branch from 2daa9e9 to 4164dd0 Compare February 27, 2018 09:01

Therzok mentioned this pull request Mar 1, 2018

Not all projects use versions from Packages.props #25148

Closed

Therzok force-pushed the sqlite-blob branch from 7c40231 to cddd680 Compare May 21, 2018 20:57

Therzok added 5 commits May 22, 2018 00:01

[SQLite] Bump and use byte[] variants added to blob_open for performance

7808c57

Fixes dotnet#23424

PR feedback

f87c0f6

Possible fix at windows build

8071e38

Fix this too

a264cd1

Fixup e_sqlite3 win rid path

bfcce60

Therzok force-pushed the sqlite-blob branch from cddd680 to bfcce60 Compare May 21, 2018 21:01

heejaechang approved these changes May 21, 2018

View reviewed changes

jinujoseph assigned jasonmalinowski May 22, 2018

Bump SQLite version to 1.1.11

a4bf10a

jinujoseph added the Approved to merge label May 29, 2018

jasonmalinowski merged commit 292e598 into dotnet:master May 29, 2018

Therzok deleted the sqlite-blob branch May 29, 2018 23:08

This was referenced Jun 5, 2018

Revert #25047 - [SQLite] Bump and use byte[] variants added to blob_open for performance #27484

Merged

SQLite assembly version need to be revised #27489

Closed

OmarTawfik added a commit that referenced this pull request Jun 6, 2018

Revert "Merge pull request #25047 from Therzok/sqlite-blob" (#27484)

717accb

Conversation

Therzok commented Feb 25, 2018

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

Therzok commented Feb 25, 2018

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!

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!

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!

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!

sharwell Feb 25, 2018 • 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!

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!

jasonmalinowski left a comment

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!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski commented Feb 26, 2018

Uh oh!

heejaechang commented Feb 26, 2018

Uh oh!

Therzok commented Feb 27, 2018

Uh oh!

Therzok commented Feb 27, 2018

Uh oh!

Therzok commented Mar 1, 2018

Uh oh!

Therzok commented Mar 1, 2018

Uh oh!

jasonmalinowski commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

sharwell Feb 25, 2018 •

edited

Loading

jasonmalinowski commented Mar 1, 2018 •

edited

Loading

heejaechang commented May 22, 2018 •

edited

Loading