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 the BBSweep header and uses #68643

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

AaronRobinsonMSFT
Copy link
Member

This helps with #68237

/cc @davidwrighton @AndyAyersMS

@AaronRobinsonMSFT
Copy link
Member Author

@davidwrighton and @AndyAyersMS A follow-up question is, can I remove all IBC code paths? If so, I will update this PR with all those changes.

@davidwrighton
Copy link
Member

@AaronRobinsonMSFT Removing this is good. For removing the IBC code paths, I approve of removing the actual IBC code, but we currently have a variety of function pairs like GetMemberDef and GetMemberDef_NoLogging. At this time, I'm not yet willing to remove all of those and consolidate down to just the GetMemberDef one. We're still a bit early in our quest to replace the old PGO infrastructure and it is still possible we may want to resurrect some of that logic, and the logging vs nologging variants of the functions represent a pragmatic set of details around what would cause performance problems if we adding an expensive logging step. However, making the function bodies of those two functions identical, or having one call the other would be fine by me.

@jkotas Do you agree?

@AaronRobinsonMSFT
Copy link
Member Author

@davidwrighton Thanks for the clarification here. That removal sounds more tricky and will likely have a fair bit of back and forth. I'll merge this in now to help with the wprintf work. Thanks.

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 11bff1a into dotnet:main Apr 28, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the remove_bbsweep branch April 28, 2022 18:49
@jkotas
Copy link
Member

jkotas commented Apr 28, 2022

@jkotas Do you agree?

Sounds good to me.

@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants