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

Remove generic methods from ILogSink. #8079

Merged
merged 2 commits into from
May 5, 2022
Merged

Conversation

grokys
Copy link
Member

@grokys grokys commented May 5, 2022

What does the pull request do?

Part of the push to remove generic virtual methods from Avalonia for performance reasons. Although this PR shouldn't affect performance, it will probably improve AOT compilation times slightly.

Generic methods were added to this interface in #3055 to prevent boxing before we added Logger.TryGet (#4135).

Given that since we added Logger.TryGet, only enabled logging levels will result in a call to the logger, we can move back to using params object[] and boxing; removing the generic interface methods.

Ran a few benchmarks and as expected this has no affect on performance in release mode as logging should not be enabled there.

Breaking changes

ILogSink has changed.

Part of the push to remove generic virtual methods from Avalonia for performance reasons.

Generic methods were added to this interface in #3055 to prevent boxing before we added `Logger.TryGet` (#4135).

Given that since we added `Logger.TryGet`, only enabled logging levels will result in a call to the logger, we can move back to using `params object[]` and boxing; removing the generic interface methods.
@grokys grokys requested a review from a team May 5, 2022 08:00
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0020221-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

Copy link
Collaborator

@MarchingCube MarchingCube left a comment

Choose a reason for hiding this comment

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

LGTM.

@maxkatz6 maxkatz6 enabled auto-merge May 5, 2022 17:17
@maxkatz6 maxkatz6 merged commit 7382a96 into master May 5, 2022
@maxkatz6 maxkatz6 deleted the refactor/log-nongeneric branch May 5, 2022 17:25
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 0.10.999-cibuild0020245-beta. (feed url: https://nuget.avaloniaui.net/repository/avalonia-all/index.json) [PRBUILDID]

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.

4 participants