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

Use function pointers instead of marshalled delegates in EventSource #79970

Merged
merged 5 commits into from
Jan 6, 2023
Merged

Use function pointers instead of marshalled delegates in EventSource #79970

merged 5 commits into from
Jan 6, 2023

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Dec 26, 2022 •
edited
Loading

Function pointers are more efficient and avoid JITing. This is one of the last remaining uses of marshalled delegates in core framework. It allows the infrastructure for marshalled delegates to be trimmed.

@ghost ghost assigned jkotas Dec 26, 2022
@jkotas jkotas requested a review from radical as a code owner December 26, 2022 07:28
@build-analysis build-analysis bot mentioned this pull request Dec 26, 2022
Closed
@build-analysis build-analysis bot mentioned this pull request Dec 26, 2022
Closed
@jkotas jkotas requested a review from noahfalk December 26, 2022 23:27
noahfalk
noahfalk approved these changes Jan 4, 2023
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

cc @davmason

davmason
davmason approved these changes Jan 4, 2023
View reviewed changes
Copy link
Member

@davmason davmason left a comment

Choose a reason for hiding this comment

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

LGTM

maraf
maraf approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member

@maraf maraf left a comment

Choose a reason for hiding this comment

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

WASM bits LGTM

@jkotas jkotas mentioned this pull request Jan 6, 2023
Closed
@jkotas jkotas merged commit 7d91fd0 into dotnet:main Jan 6, 2023
@jkotas jkotas deleted the etw-callback branch January 6, 2023 00:06
@janvorli
Copy link
Member

@jkotas this PR has broken unloadability for a couple of coreclr tests on both Windows and Unix. The unloading is blocked due to root like this (the root path differs a bit test by test):

(lldb) gcroot -all 00007fbf6a813690
HandleTable:
    00007FFFF4AC1390 (strong handle)
    -> 00007FBF6A81A800 System.Diagnostics.Tracing.EventPipeEventProvider
    -> 00007FBF6A81A828 System.Diagnostics.Tracing.EventEnableCallback
    -> 00007FBF6A81A7A8 System.Diagnostics.Tracing.EventSource+OverrideEventProvider
    -> 00007FBF6A816538 System.Threading.Tasks.ParallelEtwProvider
    -> 00007FBF6A813690 System.Reflection.LoaderAllocator

Found 1 roots.

The failing tests are:
JIT/Performance/CodeQuality/BenchmarksGame/k-nucleotide/k-nucleotide-9
JIT/Performance/CodeQuality/BenchmarksGame/spectralnorm/spectralnorm-3
JIT/Performance/CodeQuality/BenchmarksGame/mandelbrot/mandelbrot-7
JIT/Performance/CodeQuality/Roslyn/CscBench
JIT/Regression/JitBlue/GitHub_19361/GitHub_19361
JIT/superpmi/superpmicollect/CscBench
profiler/multiple/multiple
Regressions/coreclr/GitHub_45929/test45929
tracing/eventlistener/eventlistener

@janvorli
Copy link
Member

To repro this, just run the coreclr tests with "runincontext" argument added. Btw, there are few additional failed test that fail due to incompatibilities with unloading that I am working on disabling.

@janvorli
Copy link
Member

@jkotas can you please take a look when you have a chance?

@jkotas jkotas mentioned this pull request Jan 10, 2023
Closed
@jkotas
Copy link
Member Author

jkotas commented Jan 10, 2023

I have opened #80450 on this.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@davmason davmason davmason approved these changes

@maraf maraf maraf approved these changes

@noahfalk noahfalk noahfalk approved these changes

@lateralusX lateralusX Awaiting requested review from lateralusX lateralusX is a code owner

@lambdageek lambdageek Awaiting requested review from lambdageek

@vargaz vargaz Awaiting requested review from vargaz

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar

@radical radical Awaiting requested review from radical

Assignees

@jkotas jkotas

Labels
area-Tracing-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jkotas @janvorli @vargaz @lambdageek @stephentoub @noahfalk @maraf @davmason