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

Optimize capturing lambdas in DependencyContextExtensions and DependencyContextJsonReader #92462

Merged
merged 7 commits into from
Nov 28, 2023

Conversation

karakasa
Copy link
Contributor

@karakasa karakasa commented Sep 22, 2023

close #87911

I am kind of uncertain where to put the helper methods, as I don't want to pollute other namespaces but the methods' function are very generic and might be useful to others.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 22, 2023
@ghost
Copy link

ghost commented Sep 22, 2023

Tagging subscribers to this area: @dotnet/area-dependencymodel
See info in area-owners.md if you want to be subscribed.

Issue Details

close #87911

Author: karakasa
Assignees: -
Labels:

area-DependencyModel, community-contribution

Milestone: -

@karakasa karakasa marked this pull request as ready for review September 22, 2023 08:15
`DependencyContextExtensions` and `DependencyContextJsonReader`
originally use lambdas capturing local variables, resulting in unnecessary
allocations.

The PR replaces them with helper functions with static lambdas.

Fix dotnet#87911
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

It looks to me like the issue this issue attempts to address has an open question about the scenario being measured that notices/cares about the performance here. @Youssef1313 can you chime in on that? @ViktorHofer -- looks like you marked the original issue help-wanted can you have a look at this?

@Youssef1313
Copy link
Member

@ericstj I don't have much info other than seeing these allocations at the startup of an Uno Platform Skia application. From the trace screenshots posted in the issue, it seems like all Uno does is this:

https://github.com/unoplatform/uno/blob/8b88124a5ebdc61a782326d96ff32e5cba5d452a/src/Uno.UI.Runtime.Skia.Gtk/Rendering/OpenGLESRenderSurface.cs#L29-L32

Then Silk.NET calls into this code and then we see these allocations.

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 27, 2023

I'm also surprised that this code is executed on the hot path. Nevertheless, the optimization looks trivial (replacing the lamda with a loop) so I'm OK with that. As the extension helper file is only used in a single place, it makes sense to put it next to where it is used (in DependencyModel).

@ViktorHofer ViktorHofer merged commit 8435c96 into dotnet:main Nov 28, 2023
107 of 109 checks passed
@karakasa karakasa deleted the issue-87911 branch November 29, 2023 04:13
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-DependencyModel community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize allocations of a capturing lambda in GetRuntimeNativeAssets
4 participants