Skip to content

[sil-inst-opt] Improve performance of InstModCallbacks by eliminating indirect call along default callback path. #35253

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

Conversation

gottesmm
Copy link
Contributor

@gottesmm gottesmm commented Jan 4, 2021

Specifically before this PR, if a caller did not customize a specific callback
of InstModCallbacks, we would store a static default std::function into
InstModCallbacks. This means that we always would have an indirect jump. That is
unfortunate since this code is often called in loops.

In this PR, I eliminate this problem by:

  1. I made all of the actual callback std::function in InstModCallback private
    and gave them a "Func" postfix (e.x.: deleteInst -> deleteInstFunc).

  2. I created public methods with the old callback names to actually call the
    callbacks. This ensured that as long as we are not escaping callbacks from
    InstModCallback, this PR would not result in the need for any source changes
    since we are changing a call of a std::function field to a call to a method.

  3. I changed all of the places that were escaping inst mod's callbacks to take
    an InstModCallback. We shouldn't be doing that anyway.

  4. I changed the default value of each callback in InstModCallbacks to be a
    nullptr and changed the public helper methods to check if a callback is
    null. If the callback is not null, it is called, otherwise the getter falls
    back to an inline default implementation of the operation.

All together this means that the cost of a plain InstModCallback is reduced and
one pays an indirect function cost price as one customizes it further which is
better scalability.

P.S. as a little extra thing, I added a madeChange field onto the
InstModCallback. Now that we have the helpers calling the callbacks, I can
easily insert instrumentation like this, allowing for users to pass in
InstModCallback and see if anything was RAUWed without needing to specify a
callback.

… indirect call along default callback path.

Specifically before this PR, if a caller did not customize a specific callback
of InstModCallbacks, we would store a static default std::function into
InstModCallbacks. This means that we always would have an indirect jump. That is
unfortunate since this code is often called in loops.

In this PR, I eliminate this problem by:

1. I made all of the actual callback std::function in InstModCallback private
   and gave them a "Func" postfix (e.x.: deleteInst -> deleteInstFunc).

2. I created public methods with the old callback names to actually call the
   callbacks. This ensured that as long as we are not escaping callbacks from
   InstModCallback, this PR would not result in the need for any source changes
   since we are changing a call of a std::function field to a call to a method.

3. I changed all of the places that were escaping inst mod's callbacks to take
   an InstModCallback. We shouldn't be doing that anyway.

4. I changed the default value of each callback in InstModCallbacks to be a
   nullptr and changed the public helper methods to check if a callback is
   null. If the callback is not null, it is called, otherwise the getter falls
   back to an inline default implementation of the operation.

All together this means that the cost of a plain InstModCallback is reduced and
one pays an indirect function cost price as one customizes it further which is
better scalability.

P.S. as a little extra thing, I added a madeChange field onto the
InstModCallback. Now that we have the helpers calling the callbacks, I can
easily insert instrumentation like this, allowing for users to pass in
InstModCallback and see if anything was RAUWed without needing to specify a
callback.
@gottesmm gottesmm requested a review from atrick January 4, 2021 20:52
@gottesmm
Copy link
Contributor Author

gottesmm commented Jan 4, 2021

@swift-ci smoke test

Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

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

Cool, thanks!

@gottesmm gottesmm merged commit 97c7376 into swiftlang:main Jan 4, 2021
@gottesmm gottesmm deleted the pr-7d30d56fcab9d42f3788104b987f62df27540e00 branch January 4, 2021 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants