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

Add NET 8.0 target to Microsoft.Bcl.Memory package #104185

Closed
tarekgh opened this issue Jun 29, 2024 · 6 comments · Fixed by #104648
Closed

Add NET 8.0 target to Microsoft.Bcl.Memory package #104185

tarekgh opened this issue Jun 29, 2024 · 6 comments · Fixed by #104648
Assignees
Labels
area-System.Memory enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@tarekgh
Copy link
Member

tarekgh commented Jun 29, 2024

Description

.NET 9.0 introduced Base64Url, and we are providing a polyfill for this type in Microsoft.Bcl.Memory to enable its use in earlier versions of .NET. The Base64Url implementation in .NET 9.0 leverages vectorization for improved performance. However, the polyfill implementation does not use vectorization, as it is not available in older framework versions. Although vectorization is possible in .NET 8.0, Microsoft.Bcl.Memory does not currently target .NET 8.0 specifically. When using Microsoft.Bcl.Memory on .NET 8.0, the polyfill implementation without vectorization is used. This issue tracks the addition of a .NET 8.0 target to Microsoft.Bcl.Memory, which will enable vectorization as well.

A quick test indicates that the current implementation, which uses vectorization, relies on the internal members of the public Ascii class: VectorContainsNonAsciiChar, ExtractAsciiVector, StoreVectorAndZip, Load4xVector128AndUnzip, and ShuffleUnsafe. These members need to be refactored to enable their use when targeting .NET 8.0.

Reproduction Steps

Run any test app on .NET 8.0 with referencing Microsoft.Bcl.Memory.

Expected behavior

Vectorization should be used

Actual behavior

No Vectorization is used

Regression?

No

Known Workarounds

Target .NET 9.0 instead.

Configuration

.NET 8.0

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 29, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jun 29, 2024
@tarekgh tarekgh added area-System.Memory untriaged New issue has not been triaged by the area owner and removed untriaged New issue has not been triaged by the area owner needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 29, 2024
Copy link
Contributor

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

@tarekgh
Copy link
Member Author

tarekgh commented Jun 29, 2024

CC @stephentoub

@julealgon
Copy link

.NET 9.0 introduced Base64Url

@tarekgh would you mind providing a link or some minimal information about this new type? Tried searching for it on the docs page but it doesn't seem to be there yet. I see one type with that name but its in an Azure namespace:

@tarekgh
Copy link
Member Author

tarekgh commented Jul 1, 2024

@julealgon This is a brand new type, and the documentation has not yet been ported to the public portal. You can refer to the in-code documentation available at Base64Url documentation in the code. The triple-slash comments there will be the ones eventually ported to the public documentation.

CC @buyaa-n

@ericstj ericstj added the enhancement Product code improvement that does NOT require public API changes/additions label Jul 8, 2024
@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jul 8, 2024
@ericstj ericstj added this to the 9.0.0 milestone Jul 8, 2024
@ericstj
Copy link
Member

ericstj commented Jul 8, 2024

So there's a bit of a cost/benefit analysis to consider here. The current netstandard implementation works on net8.0 but could be benefit if it used vectorization. Sounds like using vectorization will require some refactoring of the code and potentially maintaining a separate copy - so that's a cost here. @buyaa-n can you review this request and discuss with stakeholders and decide if it's work the cost to improve the implementation on net8.0?

@buyaa-n
Copy link
Contributor

buyaa-n commented Jul 8, 2024

I believe it would only need a copy of the internal members of the Ascii class: VectorContainsNonAsciiChar, ExtractAsciiVector, StoreVectorAndZip, Load4xVector128AndUnzip, I'll check.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Memory enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants