Skip to content

Improve UnwindInfoTable's performance#125545

Merged
eduardo-vp merged 24 commits intodotnet:mainfrom
eduardo-vp:lock-per-UnwindInfoTableObject
Mar 20, 2026
Merged

Improve UnwindInfoTable's performance#125545
eduardo-vp merged 24 commits intodotnet:mainfrom
eduardo-vp:lock-per-UnwindInfoTableObject

Conversation

@eduardo-vp
Copy link
Copy Markdown
Member

@eduardo-vp eduardo-vp commented Mar 14, 2026

Place the entries of UnwindInfoTable in a buffer and flush such that we amortize the cost of the operations (pRtlAddGrowableFunctionTable + pRtlDeleteGrowableFunctionTable) to create the internal table of RUNTIME_FUNCTIONs.

Contributes to #123124.

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings March 14, 2026 01:29

This comment was marked as outdated.

@eduardo-vp eduardo-vp force-pushed the lock-per-UnwindInfoTableObject branch from 75cd28d to 7358ca6 Compare March 17, 2026 02:05
@eduardo-vp eduardo-vp changed the title Use a lock per UnwindInfoTable object instead of global lock Improve UnwindInfoTable's performance Mar 17, 2026
Copilot AI review requested due to automatic review settings March 18, 2026 00:47

This comment was marked as outdated.

@eduardo-vp
Copy link
Copy Markdown
Member Author

Storing in a buffer and flushing with two locks actually led to better performance of the test case mentioned in #123124 (comment).

Change Time
Main ~580 ms
Buffer + flush with 2 locks ~200 ms

Copilot AI review requested due to automatic review settings March 20, 2026 00:42
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/coreclr/vm/codeman.cpp:1

  • Removal only searches the published table and ignores entries that are still in the pending buffer. If a method is unpublished before its unwind data is flushed, the pending entry can later be published to the OS (stale unwind info), and removal will incorrectly report not found. Fix by also searching/removing matching entries from the pending buffer under s_pUnwindInfoTablePendingLock (and/or forcing a flush before attempting removal) so unpublish is correct regardless of whether the entry was already published.
// Licensed to the .NET Foundation under one or more agreements.

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Copilot AI review requested due to automatic review settings March 20, 2026 00:58
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Mar 20, 2026

What does the perf look like with the latest delta?

@eduardo-vp
Copy link
Copy Markdown
Member Author

These are the results with the latest changes

Branch Test execution time
Main ~580 ms
Current ~198 ms

Copy link
Copy Markdown
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

Two nits I'd personally like to see fix, but other than that, LGTM. Thanks!

Copilot AI review requested due to automatic review settings March 20, 2026 05:22
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

@eduardo-vp
Copy link
Copy Markdown
Member Author

/ba-g unrelated failure

@eduardo-vp eduardo-vp merged commit 43502e5 into dotnet:main Mar 20, 2026
101 of 105 checks passed
@agocke
Copy link
Copy Markdown
Member

agocke commented Mar 29, 2026

/backport to release/10.0

@github-actions
Copy link
Copy Markdown
Contributor

Started backporting to release/10.0 (link to workflow run)

@github-actions
Copy link
Copy Markdown
Contributor

@agocke backporting to release/10.0 failed, the patch most likely resulted in conflicts. Please backport manually!

git am output
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Batch unwind info table entries
Applying: Flush before method is done JITing
Applying: Use two locks
error: sha1 information is lacking or useless (src/coreclr/vm/codeman.cpp).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0003 Use two locks
Error: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

@agocke
Copy link
Copy Markdown
Member

agocke commented Mar 30, 2026

@eduardo-vp Could you port this over manually? The automation seems to be having issues.

eduardo-vp added a commit to eduardo-vp/runtime that referenced this pull request Mar 30, 2026
Place the entries of UnwindInfoTable in a buffer and flush such that we amortize the cost of the operations (pRtlAddGrowableFunctionTable + pRtlDeleteGrowableFunctionTable) to create the internal table of `RUNTIME_FUNCTION`s.

Contributes to dotnet#123124.

---------

Co-authored-by: Eduardo Velarde <evelardepola@microsoft.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants