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 some Gc holes #1296

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove some Gc holes #1296

wants to merge 1 commit into from

Conversation

benaadams
Copy link

Going via Unsafe.AsPointer risks creating Gc holes where the GC has moved the memory but the pointer has not been updates

Use safer alternatives via ref

Unsafe.CopyBlock(
ref targetPtr,
ref sourcePtr,
(uint)sourceLengthInBytes);
Copy link

@neon-sunset neon-sunset Oct 30, 2024

Choose a reason for hiding this comment

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

Cast to uint here will truncate the chunk of memory being copied to 4GiB. Instead, this could be done with pinning both byrefs and calling Buffer.MemoryCopy which does the right thing. Happy to see more people discovering an amazing project that ILGPU is.

@En3Tho
Copy link

En3Tho commented Oct 31, 2024

Related: #1209

Copy link
Owner

@m4rs-mt m4rs-mt left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR and addressing this issue. Before we merge it, could you also take a look at resolving the build issues we're seeing? Once those are addressed, this PR will be ready to move forward.

@En3Tho
Copy link

En3Tho commented Jan 17, 2025

@m4rs-mt Some build failures are related to .Net 6 runtime not having support for ref field.
.Net 6 is out of support currently. Do you think ILGpu should support it moving forward?

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.

None yet

4 participants