Avoid captures in SqlConnection.ReadBlob#36127
Conversation
This change produced a 806MB (11.3%) allocation reduction in the scenario described by dotnet#36114.
|
Aren't these changes mostly in #34790? |
|
@Therzok Yes, but I resubmitted them with finer granularity and data against the standardized test. There are also some minor style changes so it matches Roslyn, and a couple of changes that did not make a significant difference were excluded to ensure the reviews are focused on the items of interest. |
|
@Therzok It wasn't my intent to duplicate your change. More a side effect of trying to document the baseline values and validate the incremental improvements matched the claims. In the end, it certainly appears that the new test is an accurate representation of the scenario you wanted to improve, since we arrived at very similar conclusions. In hindsight, I'd actually be a bit concerned if these PRs didn't have significant overlap with yours. |
|
Oh, no worries, I was just thinking the commits would be cherry-pickable, but after looking at my PR again, they touch multiple things in the 'address PR feedback' commits. So looks good to me! |
| public void RunInTransaction<TState>(Action<TState> action, TState state) | ||
| { | ||
| RunInTransaction( | ||
| state => |
There was a problem hiding this comment.
fwiw, calling this state was super confusing.
There was a problem hiding this comment.
😕 I thought that was the normal name for this. Examples of alternatives?
There was a problem hiding this comment.
it's more that the outer param for RunInTransaction is called "state" and hte inner one is called "state" and my brain exploded with confusion. Honestly, i'd be 100% fine with you calling the inner one tuple :)
There was a problem hiding this comment.
it's also a nit. if you're ok with this, it's NBD. just giving you my stream of consciousness thoughts as i tried to grok things :)
There was a problem hiding this comment.
I've seen other instances of this pattern with TArg and arg.
There was a problem hiding this comment.
Note: the pattern is fine. it's literally the fact that there are two params called 'state', and then the code has state.state :D It's purely naming i don't like. arg or tuple would be great IMO just for some simple clarity.
src/Workspaces/Core/Portable/Storage/SQLite/SQLitePersistentStorage_StringIds.cs
Show resolved
Hide resolved
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Approve
Submit feedback approving these changes.
This change produced a 806MB (11.3%) allocation reduction in the scenario described by #36114.