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

Disable runtime marshalling for the CsWinRT repo. #1431

Conversation

jlaanstra
Copy link
Collaborator

Disable runtime marshalling for the CsWinRT repo.

@Sergio0694
Copy link
Member

Just so I understand, is this to start catching all remaining cases and address them? Because I can't see things just working with no changes at all after enabling this flag, especially given I know there's still other PRs removing more marshalling stuff 😅

Either way, absolutely love this, and it was about time! 🚀

src/Directory.Build.props Outdated Show resolved Hide resolved
@Sergio0694 Sergio0694 added performance Related to performance work AOT labels Jan 8, 2024
@manodasanW
Copy link
Member

Just so I understand, is this to start catching all remaining cases and address them? Because I can't see things just working with no changes at all after enabling this flag, especially given I know there's still other PRs removing more marshalling stuff 😅

Either way, absolutely love this, and it was about time! 🚀

The goal is to see what is going to break. But I am curious which marshaling thing you are referring to?

@Sergio0694
Copy link
Member

"The goal is to see what is going to break."

Gotcha, yeah that makes perfect sense 👍

"I am curious which marshaling thing you are referring to?"

I was thinking of those in #1416, plus I expect some more stuff will also break. For instance:

  • Some Marshal calls here and there, don't think all of them will work with disabled marshalling
  • Same as above, specifically in the As<T>() code checking for COM import interfaces. On a related note, we should likely also do a check for NAOT and just skip that entirely (or throw), because NAOT just doesn't support built-in COM import at all.

@Sergio0694
Copy link
Member

Unrelated, it seems like the CI isn't running? Is this because the source branch is from another fork?
I remember Jeremy also had to close his PR and re-open it from a branch in this same repo (for #1421).

@manodasanW
Copy link
Member

Unrelated, it seems like the CI isn't running? Is this because the source branch is from another fork? I remember Jeremy also had to close his PR and re-open it from a branch in this same repo (for #1421).

Right, we don't automatically run CI for forks.

@manodasanW
Copy link
Member

"The goal is to see what is going to break."

Gotcha, yeah that makes perfect sense 👍

"I am curious which marshaling thing you are referring to?"

I was thinking of those in #1416, plus I expect some more stuff will also break. For instance:

  • Some Marshal calls here and there, don't think all of them will work with disabled marshalling
  • Same as above, specifically in the As<T>() code checking for COM import interfaces. On a related note, we should likely also do a check for NAOT and just skip that entirely (or throw), because NAOT just doesn't support built-in COM import at all.

That PR was trying to port the changes already in the staging branch to master. The staging branch should already have blittable signatures. It would be interesting to see which Marshal functions break when the tests run.

@jlaanstra
Copy link
Collaborator Author

Some tests use runtime marshalling which is fine, so I scoped it down to actual artifacts produced by this repo.

@jlaanstra jlaanstra requested a review from Sergio0694 January 9, 2024 21:27
@Sergio0694
Copy link
Member

The changes look good to me, but I have to say I feel like I'm missing something here, or maybe I just don't completely understand what does disabling runtime marshalling affect. We have several uses of stuff like Marshal.StructureToPtr in CsWinRT, as well as some delegates with managed signatures, which we're using to get native function pointers for (eg. we do this in the ABI Nullable<T> type. I'm not sure I get how can eg. all of that continue to work with the changes in this PR 🤔

@jlaanstra
Copy link
Collaborator Author

The changes look good to me, but I have to say I feel like I'm missing something here, or maybe I just don't completely understand what does disabling runtime marshalling affect. We have several uses of stuff like Marshal.StructureToPtr in CsWinRT, as well as some delegates with managed signatures, which we're using to get native function pointers for (eg. we do this in the ABI Nullable<T> type. I'm not sure I get how can eg. all of that continue to work with the changes in this PR 🤔

This change is here to find all remaining cases where we rely on runtime marshalling with the goal of eventually not having any.

@Sergio0694
Copy link
Member

No I know that. I think I got tripped up by the CI being green, that's the part that was not adding up.
Then I remembered that this PR isn't actually triggering the CI, so ok it all makes sense now 😄

Question: would it not be better not to merge this PR though, to avoid completely breaking the CI in the branch until we fix everything? As in, we can just keep rebasing this one as it spots more issues, and then we can gradually fix them in other PRs, and only merge this one once the CI for this one is actually all green? And on that note, can't we close and reopen this from a branch in this same repo like I mentioned, to get the CI to actually run from this branch?

@jlaanstra
Copy link
Collaborator Author

No I know that. I think I got tripped up by the CI being green, that's the part that was not adding up. Then I remembered that this PR isn't actually triggering the CI, so ok it all makes sense now 😄

Question: would it not be better not to merge this PR though, to avoid completely breaking the CI in the branch until we fix everything? As in, we can just keep rebasing this one as it spots more issues, and then we can gradually fix them in other PRs, and only merge this one once the CI for this one is actually all green? And on that note, can't we close and reopen this from a branch in this same repo like I mentioned, to get the CI to actually run from this branch?

A manually kicked off build is actually green right now.

@MartyIX
Copy link

MartyIX commented Mar 16, 2024

What performance improvement can this bring? Is there at least a ballpark estimate?

@Sergio0694
Copy link
Member

No performance difference whatsoever.

@jlaanstra jlaanstra force-pushed the user/jlaans/disable-runtime-marshalling branch from cfc6fce to 7c3623f Compare April 11, 2024 18:51
@jlaanstra jlaanstra merged commit 91fca8c into microsoft:staging/AOT Apr 19, 2024
1 check passed
@jlaanstra jlaanstra deleted the user/jlaans/disable-runtime-marshalling branch April 19, 2024 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AOT performance Related to performance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants