Skip to content

Use function pointers instead of marshalled delegates in EventSource#79970

Merged
jkotas merged 5 commits intodotnet:mainfrom
jkotas:etw-callback
Jan 6, 2023
Merged

Use function pointers instead of marshalled delegates in EventSource#79970
jkotas merged 5 commits intodotnet:mainfrom
jkotas:etw-callback

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 added the area-Tracing-coreclr label Dec 26, 2022
@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 commented Dec 26, 2022
View reviewed changes
@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

jkotas
jkotas commented Jan 4, 2023
View reviewed changes
davmason
davmason approved these changes Jan 4, 2023
View reviewed changes
Copy link
Contributor

@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

janvorli commented Jan 10, 2023

@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

janvorli commented Jan 10, 2023

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

janvorli commented Jan 10, 2023

@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

@noahfalk noahfalk noahfalk approved these changes

@maraf maraf maraf 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

+1 more reviewer

@davmason davmason davmason approved these changes

Reviewers whose approvals may not affect merge requirements

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