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.

Sign up for GitHub

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

Jump to bottom

Improve SqliteDataRecord class Efficiency #18438

Merged
merged 2 commits into from
Feb 26, 2020
Merged

Improve SqliteDataRecord class Efficiency #18438

merged 2 commits into from
Feb 26, 2020

Conversation

nmichels
Copy link
Contributor

@dnfclas
Copy link

dnfclas commented Oct 18, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

@bricelam bricelam self-assigned this Oct 18, 2019
@bricelam bricelam added this to the 5.0.0 milestone Oct 25, 2019
@bricelam bricelam removed this from the 5.0.0 milestone Jan 28, 2020
bricelam
bricelam suggested changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

Another part of #13987 is to cache rowidOrdinal so we don't have to find it every time we call GetStream. (remember to invalidate the cache in Read)

src/Microsoft.Data.Sqlite.Core/SqliteDataRecord.cs Outdated Show resolved Hide resolved
src/Microsoft.Data.Sqlite.Core/SqliteDataRecord.cs Outdated Show resolved Hide resolved
@bricelam
Copy link
Contributor

Oh, can you override DbDataReader.GetTextReader to return new StreamReader(GetStream(ordinal), Encoding.UTF8) too?

@nmichels
Copy link
Contributor Author

Thanks @bricelam
I will provide the requested changes as soon as possible!

@bricelam bricelam force-pushed the ImproveSqliteDataRecordEfficiency branch from 1bf20bc to dcf2776 Compare February 20, 2020 23:41
bricelam added a commit to nmichels/EntityFrameworkCore that referenced this pull request Feb 20, 2020
@bricelam
Clean up PR dotnet#18438 and add some tests
dcf2776
bricelam added a commit to nmichels/EntityFrameworkCore that referenced this pull request Feb 20, 2020
@bricelam
Clean up PR dotnet#18438 and add some tests
8578769
@bricelam bricelam force-pushed the ImproveSqliteDataRecordEfficiency branch from dcf2776 to 8578769 Compare February 20, 2020 23:45
@bricelam bricelam removed the accepted label Feb 20, 2020
bricelam
bricelam suggested changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

Oh wait, still need to cache rowidOrdinal.

@nmichels
Copy link
Contributor Author

@bricelam I've commited some changes related to caching rowidOrdinal just to check if the general approach is OK, in case it is, I can perform a code clean up and add more tests.

bricelam added a commit to nmichels/EntityFrameworkCore that referenced this pull request Feb 26, 2020
@bricelam
Clean up PR dotnet#18438 and add some tests
10ce870
@bricelam bricelam force-pushed the ImproveSqliteDataRecordEfficiency branch from 06f0bb6 to 10ce870 Compare February 26, 2020 22:22
bricelam added a commit to nmichels/EntityFrameworkCore that referenced this pull request Feb 26, 2020
@bricelam
Clean up PR dotnet#18438 and add some tests
9c540ab
@bricelam bricelam force-pushed the ImproveSqliteDataRecordEfficiency branch from 10ce870 to 9c540ab Compare February 26, 2020 22:43
nmichels and others added 2 commits February 26, 2020 14:46
@nmichels @bricelam
Improve SqliteDataRecord class Efficiency
84611fb 
- Update methods GetBytes and GetChars to use internally GetStream.
@bricelam
Clean up PR dotnet#18438 and add some tests
5c5227b
@bricelam bricelam force-pushed the ImproveSqliteDataRecordEfficiency branch from 9c540ab to 5c5227b Compare February 26, 2020 22:47
@bricelam bricelam merged commit 5c5227b into dotnet:master Feb 26, 2020
@bricelam
Copy link
Contributor

Thanks for the contribution @nmichels! Sorry if I did too much; I probably should have pinged you before picking it up where you left off.

@nmichels nmichels deleted the ImproveSqliteDataRecordEfficiency branch February 26, 2020 23:20
@smitpatel
Copy link
Member

I believe after this PR only, I am seeing intermittent seg fault on sqlite tests on helix.
See https://helix.dot.net/api/2019-06-17/jobs/747f76dc-fa0f-4920-ace5-09f8351ce3b7/workitems/Microsoft.Data.Sqlite.Tests.dll/console

smitpatel added a commit that referenced this pull request Feb 27, 2020
@smitpatel
Revert "Clean up PR #18438 and add some tests"
47a2373 
This reverts commit 5c5227b.
smitpatel added a commit that referenced this pull request Feb 27, 2020
@smitpatel
Revert "Clean up PR #18438 and add some tests"
b416e9d 
This reverts commit 5c5227b.
@ajcvickers ajcvickers mentioned this pull request Mar 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfoidl gfoidl gfoidl left review comments

@bricelam bricelam bricelam approved these changes

Assignees

@bricelam bricelam

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Microsoft.Data.Sqlite: Make GetBytes, GetChars, and GetTextReader more efficient
5 participants
@nmichels @dnfclas @bricelam @smitpatel @gfoidl