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

Faster EventSource attribute lookup #45621

Merged
merged 4 commits into from
Jan 22, 2021
Merged

Faster EventSource attribute lookup #45621

merged 4 commits into from
Jan 22, 2021

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Dec 4, 2020 •
edited
Loading

From #45121 (comment)

Contributes to #44598

@benaadams benaadams mentioned this pull request Dec 4, 2020
Closed
@ghost
Copy link

ghost commented Dec 5, 2020

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

From #45121 (comment)

Author: benaadams
Assignees: -
Labels:

area-System.Diagnostics.Tracing, area-Tracing-coreclr
Milestone: -

@benaadams benaadams force-pushed the eventsource-attributes branch 3 times, most recently from 6216165 to bdf3bf4 Compare December 8, 2020 05:06
@runfoapp runfoapp bot mentioned this pull request Dec 8, 2020
Closed
@runfoapp runfoapp bot mentioned this pull request Dec 8, 2020
Closed
@stephentoub
Copy link
Member

Do you have data on the impact of this?

noahfalk
noahfalk approved these changes Jan 7, 2021
View reviewed changes
Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Ben!

@benaadams
Copy link
Member Author

Do you have data on the impact of this?

Calling into the runtime's MemberInfo.IsDefined by default

image

Drops the allocations when the method is called

From:
image

To:
image

As it no longer allocates since #44694

stephentoub
stephentoub reviewed Jan 15, 2021
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs Outdated
EventManifestOptions flags = EventManifestOptions.None)
{
// AllowEventSourceOverride is an option that allows either Microsoft.Diagnostics.Tracing or
// System.Diagnostics.Tracing EventSource to be considered valid. This should not matter anywhere but in Microsoft.Diagnostics.Tracing (nuget package).
Copy link
Member

Choose a reason for hiding this comment

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

Then should it be ifdef'd out of corelib?

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied it from GetCustomAttributeHelper just below; not sure about what it means in practice?

@runfoapp runfoapp bot mentioned this pull request Jan 15, 2021
Closed
@benaadams
Copy link
Member Author

Rebased to restart CI

benaadams and others added 4 commits January 20, 2021 14:52
@benaadams
Use faster single attribute lookup for Eventsource
394e5e3
@benaadams @eerhardt
Update src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tr…
52bbce3 
…acing/EventSource.cs

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@benaadams
Add debug assert for type
9790c20
@benaadams @stephentoub
Apply suggestions from code review
c47a557 
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@benaadams
Copy link
Member Author

Aside: though this has 2 approvals and all tests passed, I can't merge as don't have commit rights

@stephentoub stephentoub merged commit 061e0aa into dotnet:master Jan 22, 2021
@benaadams benaadams deleted the eventsource-attributes branch January 22, 2021 19:03
@stephentoub stephentoub mentioned this pull request Jan 23, 2021
Closed
36 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Feb 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jnyrup jnyrup jnyrup left review comments

@eerhardt eerhardt eerhardt left review comments

@tommcdon tommcdon tommcdon left review comments

@stephentoub stephentoub stephentoub approved these changes

@noahfalk noahfalk noahfalk approved these changes

@sywhang sywhang Awaiting requested review from sywhang

@josalem josalem Awaiting requested review from josalem

Assignees

@noahfalk noahfalk

Labels
area-System.Diagnostics.Tracing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@benaadams @stephentoub @jnyrup @noahfalk @eerhardt @tommcdon @jkotas @Dotnet-GitSync-Bot