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

[API Proposal]: Add CollectionsMarshal.AsMemory(List<T>) #101914

Closed
Neme12 opened this issue May 6, 2024 · 8 comments
Closed

[API Proposal]: Add CollectionsMarshal.AsMemory(List<T>) #101914

Neme12 opened this issue May 6, 2024 · 8 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections

Comments

@Neme12
Copy link

Neme12 commented May 6, 2024

Background and motivation

I previously proposed this in #90141 and it was rejected for the reason that it would prevent List<T>'s backing store from ever being changed from a T[] to some new type of array, like some sort of resizable array. Or rather, it wouldn't prevent such a change, but it would require the returned Memory<T> to now be backed by a MemoryManager, which is an additional allocation.

@stephentoub said:

While AsSpan(List) baked in the notion of List being backed by contiguous memory, AsMemory(List) would effectively bake in the notion of it being backed by a T[]. That's something we'd like to avoid doing right now. (Such an AsMemory could return a Memory backed by a MemoryManager, but that would then either require that AsMemory to start allocating or it would require the list to grow to be able to cache the MemoryManager reference, neither of which we'd want.)

However, after thinking about this some more, I don't think this is the case. Unfortunately, I cannot add a comment to the original proposal, so I'm opening a new issue.

Memory<T> can be backed by a GC object that stores the data inline in the object - currently (afaik), that's only T[] and string. But if there is ever a new type of array added to the runtime, like a resizable array, then Memory<T> could just as well be backed by this new type of array - it would store a reference to this new array-like object. Or even if List<T> is ever specialized by the runtime and changed to store the data inline in the object, like string does, as opposed to going through an indirection, then the Memory<T> could store the reference to the List<T> itself. That's why I don't think returning Memory<T> would bake in the notion of it being backed by a T[]. It would only bake in the notion of it being backed by a GC object. Such a GC object could very well be a new kind of array, or even the list itself.

Am I wrong in this conclusion?

@Neme12 Neme12 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 6, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

@stephentoub
Copy link
Member

But if there is ever a new type of array added to the runtime, like a resizable array, then Memory could just as well be backed by this new type of array - it would store a reference to this new array-like object.

That's not pay for play. It would make existing use of members like memory.Span more expensive.

I don't think anything meaningful has changed since the last time this was discussed and rejected.

@huoyaoyuan
Copy link
Member

#87210

@Neme12
Copy link
Author

Neme12 commented May 6, 2024

That's not pay for play. It would make existing use of members like memory.Span more expensive.

How so? In this snippet, ObjectHasComponentSize would return true for a hypothetical new kind of array. And this new kind of array could store Length in the same location in memory as a regular array, so getting it could use the same code.

else if (RuntimeHelpers.ObjectHasComponentSize(tmpObject))
{
// We know the object is not null, it's not a string, and it is variable-length. The only
// remaining option is for it to be a T[] (or a U[] which is blittable to T[], like int[]
// and uint[]). As a special case of this, ROM<T> allows some amount of array variance
// that Memory<T> disallows. For example, an array of actual type string[] cannot be turned
// into a Memory<object> or a Span<object>, but it can be turned into a ROM/ROS<object>.
// We'll assume these checks succeeded because they're performed during Memory<T> construction.
// It's always possible for somebody to use private reflection to bypass these checks, but
// preventing type safety violations due to misuse of reflection is out of scope of this logic.
// 'tmpObject is T[]' below also handles things like int[] <-> uint[] being convertible
Debug.Assert(tmpObject is T[]);
refToReturn = ref MemoryMarshal.GetArrayDataReference(Unsafe.As<T[]>(tmpObject));
lengthOfUnderlyingSpan = Unsafe.As<T[]>(tmpObject).Length;
}

I think it would be a real shame if some new kind of resizable array was added, and it had no AsMemory() method. These days, I would personally consider that a requirement for any kind of new array type, along with AsSpan().

@Neme12
Copy link
Author

Neme12 commented May 6, 2024

But okay, it seems there is no interest in this. I'll just keep using my own implementation of this. Thanks for entertaining the idea.

@Neme12 Neme12 closed this as not planned Won't fix, can't repro, duplicate, stale May 6, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label May 6, 2024
@theodorzoulias
Copy link
Contributor

Unfortunately, I cannot add a comment to the original proposal, so I'm opening a new issue.

I don't think that the goddess Fortuna has anything to do with this. Locking GitHub issues after a month of inactivity is a deliberate policy implemented by Microsoft. It didn't just occurred by accident. The reason that this policy exists continues to baffle me. As far as I can think, it only serves at depriving Microsoft from feedback about their products, from the knowledgeable engineers who use them.

@Neme12
Copy link
Author

Neme12 commented May 8, 2024

@theodorzoulias It's only here as far as I can tell, not Microsoft in general. I had been contributing to Roslyn quite a bit in the past, and they never had any policy like this (and they still do not). I was fixing issues no matter how old.

@danmoseley
Copy link
Member

danmoseley commented May 8, 2024

@theodorzoulias

As far as I can think, it only serves at depriving Microsoft from feedback about their products, from the knowledgeable engineers who use them.

I believe the bot locks closed issues and PRs after a month. Whereas open issues are only closed after 4-5 years of inactivity, and even then only after a notice saying that any comment will restart the clock for another several years.

Locking closed issues is not done to avoid community comments. It is to direct them to open issues so that we actually see the comments. Before this we would often get comments on closed issues which we would not see, as practically we only monitor open issues. Often times those comments should have gone in a new issue anyway - for example the original problem was fixed, and they are experiencing the same symptoms for a different reason. I don't think this policy of locking closed issues is unique to this repo.

If it really makes sense to reopen an issue rather than start a new one, what I suggest is asking for a maintainers to do that using the Discord channel or by creating a discussion topic here or possibly asking in a related issue.

Does that help? This is not about suppressing comment.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections
Projects
None yet
Development

No branches or pull requests

5 participants