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

c#: Remove Copy of data during import call for base types #1122

Merged
merged 3 commits into from
Jan 16, 2025

Conversation

jsturtevant
Copy link
Collaborator

When calling an import we don't need to copy data to a new structure for canonical types. This avoid the extra copy of data in this scenario.

This isin prep for some more changes with #1080

…for cannoncal types. This avoid the extra copy of data in this scenario

Signed-off-by: James Sturtevant <[email protected]>
@@ -1063,7 +1064,7 @@ impl Bindgen for FunctionBindgen<'_, '_> {

match direction {
Direction::Import => {
let import_name = self.interface_gen.type_name_with_qualifier(&Type::Id(id), true);
let import_name = self.interface_gen.type_name_with_qualifier(&Type::Id(id), true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some how these two lines had tabs, while rest of the file was spaces

Using a span and fixed keyword won't work with variants due to the fact that the external import call requires different types. Nesting of the fixed commands also become unwiedly

Signed-off-by: James Sturtevant <[email protected]>
@alexcrichton alexcrichton requested a review from dicej January 15, 2025 21:06
Copy link
Collaborator

@dicej dicej left a comment

Choose a reason for hiding this comment

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

LGTM modulo minor spelling and punctuation nits. Thanks!

crates/csharp/src/function.rs Outdated Show resolved Hide resolved
crates/csharp/src/function.rs Outdated Show resolved Hide resolved
crates/csharp/src/function.rs Outdated Show resolved Hide resolved
@dicej dicej added this pull request to the merge queue Jan 16, 2025
@jsturtevant
Copy link
Collaborator Author

A quick note for myself in the future, I did run some non scientific tests to compare speed of the various implementations:

  • using stackalloc (previous method)
  • using the ptr to the array (method implemented here)

The speed of execution difference varied but was never more than a few microseconds difference. The biggest and probably more important change here is the extra copy and allocation isn't happening on the stack. With larger byte arrays would cause a memory exception. With this change large byte arrays are not an issue.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 16, 2025
@jsturtevant
Copy link
Collaborator Author

failure doesn't look related: @github-actions[bot] Test (macos-latest, teavm-java)

@jsturtevant jsturtevant added this pull request to the merge queue Jan 16, 2025
Merged via the queue into bytecodealliance:main with commit b0f6fd8 Jan 16, 2025
25 checks passed
@jsturtevant jsturtevant deleted the avoid-unnessary-copy branch January 16, 2025 01:49
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