Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add CollectionsMarshal ref assemblies#41306

Merged
safern merged 6 commits intodotnet:masterfrom
benaadams:CollectionsMarshal
Oct 3, 2019
Merged

Add CollectionsMarshal ref assemblies#41306
safern merged 6 commits intodotnet:masterfrom
benaadams:CollectionsMarshal

Conversation

@benaadams
Copy link
Member

@benaadams benaadams commented Sep 25, 2019

Resolves dotnet/corefx#31597

/cc @tannergooding @ahsonkhan

This was referenced Sep 25, 2019
Merged
Merged
ahsonkhan
ahsonkhan reviewed Sep 25, 2019
View reviewed changes
src/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs Outdated
public ComAliasNameAttribute(string alias) { }
public string Value { get { throw null; } }
}
public static class CollectionsMarshal
Copy link

@ahsonkhan ahsonkhan Sep 25, 2019

Choose a reason for hiding this comment

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

nit: add partial

We should also try to auto-generate this if possible.

@safern might be able to help provide guidance here on how for local build of coreclr (outside of https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/updating-ref-source.md)

stephentoub
stephentoub reviewed Sep 25, 2019
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for addingthis.

What about tests?

Also, presumably there are places we should be using this in corefx? I don't love adding API we can't/don't ourselves use.

@benaadams benaadams force-pushed the CollectionsMarshal branch 2 times, most recently from eaf3f9f to c8520c2 Compare September 25, 2019 19:33
@benaadams
Copy link
Member Author

benaadams commented Sep 25, 2019 •
edited
Loading

What about tests?

Added

Also, presumably there are places we should be using this in corefx?

Yes though; it will be easier to address that when corefx/coreclr are in sync (post merge).

As an aside this

foreach (T item in CollectionsMarshal.AsSpan(list))
{
    // ...
}

would be faster than this

foreach (T item in list)
{
    // ...
}

for larger lists and same for for loops (as the later has additional bounds checks and enumerator; while the former does not); not sure that would be a desirable change to make?

ahsonkhan
ahsonkhan reviewed Sep 25, 2019
View reviewed changes
@benaadams
Copy link
Member Author

benaadams commented Sep 26, 2019

Applied feedback

@benaadams benaadams requested a review from ahsonkhan September 26, 2019 16:32
ahsonkhan
ahsonkhan approved these changes Sep 26, 2019
View reviewed changes
Copy link

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Looks good.

@safern
Copy link
Member

safern commented Oct 3, 2019

CoreCLR was consumed yesterday. Let's re-run CI.

@safern
Copy link
Member

safern commented Oct 3, 2019

/azp run corefx-ci

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 3, 2019

Azure Pipelines successfully started running 1 pipeline(s).

benaadams and others added 5 commits October 3, 2019 19:32
@benaadams
Copy link
Member Author

benaadams commented Oct 3, 2019 

 The type or namespace name 'List<>' does not exist in the namespace 'System.Collections.Generic' (are you missing an assembly reference?)

Need to check what I should include in the ref assembly

@safern safern merged commit 74cfdad into dotnet:master Oct 3, 2019
@safern
Copy link
Member

safern commented Oct 3, 2019

Thanks @benaadams now that this is merge are you planning on updating corefx to use this API?

@benaadams benaadams deleted the CollectionsMarshal branch October 3, 2019 20:49
@benaadams
Copy link
Member Author

benaadams commented Oct 4, 2019

are you planning on updating corefx to use this API?

Yes, however I have quite a few open PRs that need cleaning up and also am currently short on free time, so it may be a while.

One nice feature (and where InteropServices fits) is getting a pointer and length from a List<T> to then pass into a native call.

As an aside, @stephentoub would this be a dubious performance optimisation? #41306 (comment)

@stephentoub
Copy link
Member

stephentoub commented Oct 4, 2019

As an aside, @stephentoub would this be a dubious performance optimisation? #41306 (comment)

I'd expect the IL to be better, and the generated asm. Is there any situation/condition where it would result in worse code or perf in a microbenchmark (maybe because of the null check if that doesn't get elided)? If not, then it seems reasonable to employ in places where we think there's a reasonable gain to be had, where we expect to be enumerating long lists with minimal work done per element. Where did you have in mind?

@benaadams
Copy link
Member Author

benaadams commented Oct 4, 2019

Added a few examples in coreclr dotnet/coreclr#27032

@benaadams benaadams mentioned this pull request Oct 4, 2019
Closed
@karelz karelz added this to the 5.0 milestone Dec 19, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@benaadams @safern
Add CollectionsMarshal ref assemblies (dotnet/corefx#41306)
0e68a2f 
* Add CollectionsMarshal

* Feedback

* nits

* Feedback

* Update src/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/CollectionsMarshalTests.cs

Co-Authored-By: Ahson Khan <ahkha@microsoft.com>

* Reference collections


Commit migrated from dotnet/corefx@74cfdad
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@stephentoub stephentoub stephentoub left review comments

@ahsonkhan ahsonkhan ahsonkhan approved these changes

@safern safern safern approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Runtime.InteropServices

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

6 participants

@benaadams @safern @stephentoub @ahsonkhan @karelz @Dotnet-GitSync-Bot