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.

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

Merge TDSParserStateObject.StateSnapshot stateObj fields to a new class in shared #2157

Merged
merged 1 commit into from Sep 20, 2023

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Sep 15, 2023

There are a number of fields which are used to store data about the state object that are captured and then restored as the snapshot is replayed. Moving these fields to a new object has two benefits. 1) the maintenance of the packet list and the state is separated logically making both easier to understand and 2) if we introduce a continue option to improve performance we will need to be able to store stateobject state for both the replay start and the replay continue points.

@codecov
Copy link

codecov bot commented Sep 16, 2023

@Wraith2
Copy link
Contributor Author

Wraith2 commented Sep 17, 2023

The pr that builds on this one, combine-32, has a ~10% async string perf increase at 1mb so it'd be nice to get this one in sooner rather than later.

Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

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

LGTM

@David-Engel
Copy link
Contributor

David-Engel commented Sep 19, 2023

Just a couple comments. Since this is touching core code, I did look and see the few uncovered lines. Now I know what null bitmaps are. 😄 Interesting to see it appears that existing tests aren't covering those lines.

I also learned earlier this year that ALTMETADATA is related to functionality that was removed from SQL Server long ago. It's probably candidate code for pruning since we would never be able to hit that code against current server versions.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Sep 20, 2023

I've seen null bitmaps around the docs but never knew what alt metadata was about either. I'll put it on my list of things to prune.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Sep 20, 2023

@JRahnama @DavoudEshtehari @arellegue any chance of a quick review?

@David-Engel David-Engel merged commit 111033e into dotnet:main Sep 20, 2023
132 checks passed
@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 1, 2023

@Wraith2 What happended to combine-32 ??

@Wraith2
Copy link
Contributor Author

Wraith2 commented Dec 1, 2023

It was merged,. here #2164

@ErikEJ
Copy link
Contributor

ErikEJ commented Dec 1, 2023

@Wraith2 Thanks, managed to uncover it!

@Wraith2 Wraith2 deleted the combine-31 branch March 17, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants