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 unnecessary NoInlining, #931 #1099

Merged
merged 1 commit into from
Jan 12, 2025
Merged

Conversation

paulirwin
Copy link
Contributor

@paulirwin paulirwin commented Jan 12, 2025

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Remove unnecessary use of [MethodImpl(MethodImplOptions.NoInlining)]

Partial #931

Description

I need to finish reviewing the methods mentioned in calls to StackTraceHelper.DoesStackTraceContainMethod(string methodName) (the overload without the class name parameter) to make sure there aren't any false positives there, but I've at least validated that those existing usages of NoInlining seem like they might plausibly match their use in the tests.

This PR removes NoInlining from interface and abstract methods, where it has no effect because the attribute is not inherited. It also removes it from methods with empty bodies, because an empty body doesn't call anything, and thus those checks in the tests would never be true anyways. Finally, it removes it in a few other places that were not asserted in tests.

As a TODO, I left the SystemConsole attributes there, even though those are suspect. We had discussed possibly being able to remove that class anyways, but I'll defer that for now.

@paulirwin paulirwin added the notes:performance-improvement Contains a performance improvement label Jan 12, 2025
Copy link
Contributor

@NightOwl888 NightOwl888 left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good to merge.

@NightOwl888
Copy link
Contributor

Oh, I didn't notice it is still a draft. Feel free to re-request a review when the PR is ready.

@paulirwin paulirwin marked this pull request as ready for review January 12, 2025 16:35
@paulirwin
Copy link
Contributor Author

I'll go ahead and split out the remaining investigation work into its own PR, and merge this one as it's a nice seam.

@paulirwin paulirwin merged commit 84b7c60 into apache:master Jan 12, 2025
267 checks passed
@paulirwin paulirwin deleted the issue/931 branch January 12, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:performance-improvement Contains a performance improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants