-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Intrinsicify (Sse, Axv2, Arm64, wasm) JsonReaderHelper.IndexOfOrLessThan #41097
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that many of our libraries that are inbox and OOB do this:
This means that
NetCoreAppCurrentis the build that goes in the runtimepack/shared framework. But in the NuGet package, onlynetstandard2.0;netcoreapp3.0;net461builds get packaged in there. So that means if someone references the OOB System.Text.Json v7 NuGet package, even onnet6.0, they will use thenetcoreapp3.0version from the package, because that's the highest version available.So I really think these conditions should be
'$(TargetFramework)' == '$(NetCoreAppCurrent)' or '$(TargetFramework)' == 'netcoreapp3.0'- Include.Intrinsics. Else - include.NoIntrinsics.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intrinsics will break on netcoreapp3.0 though because they include the arm variants? Should I include
net5.0and treat that as the netcoreapp3.0 in your example?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs the experts.
@ericstj @steveharter @layomia - how do you want to handle Arm intrinsics in STJ? Do you want to ship a
net5.0TFM in the OOB package?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could type forward, but not sure how that works either 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type forwards wouldn't work here. I assume to support Arm we will need a new TFM in the package. But I think that's up to the System.Text.Json owners to decide. I'm just a fly-by reviewer 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily, I might build self-contained and run cross-gen myself. Or I might be running on an aot runtime.
That's not always an option. Consider what happens in .NET 7.0. We're going to add API to this library and I bet the Azure SDK (or some other libs) will want to use it. At that point any 6.0 app that uses Azure SDK will have no choice but to use the package version.
I'd prefer we keep a simple rule here. If the configuration differs in conditions/ifdefs from the packaged configurations then it should also be included in the package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will continue to work - with small perf hit - even if we keep the status quo.
It is ok with me if we want build extra targets to ensure that newer OOB packages on downlevel platforms always get all performance tweaks possible.
There are other assemblies with this problem. For example,
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Encodings.Web/src/System.Text.Encodings.Web.csproj uses ARM intrisics inbox, but there is no OOB build with ARM intrinsics. I assume that we would want to fix all these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like an issue to resolve before next year's 7.0 release? 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing gets resolved by that point. New API in the package means people will use that new API, so consuming the package that's newer than the framework is super common. Folks shouldn't have to pay a penalty when they do that. My position on this is don't use
ExcludeCurrentNetCoreAppFromPackageif there is anything different about theNetCoreAppCurrentbuild. Don't split hairs on something saying it's OK because it's only a small perf fix. You might be able to convince yourself of that, but what about the next person that doesn't notice it's excluded? What about the customer that cares about that perf-fix.The package growth problem is less of an issue now that we're starting to drop out of support frameworks from packages thanks to precedent that @ViktorHofer is setting.
Let's keep our rules simple: if you if'def/conditionally compile, then don't exclude from package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the initial feedback, please don't set
ExcludeCurrentNetCoreAppFromPackageanymore as we don't want to exclude the latest .NETCoreApp asset in packages anymore. See #53439 for more details.As @ericstj mentioned, in the past we excluded specific tfms - even though they were shipping in some form - from our packages but based on the new package support policy we don't carry along unsupported .NETCoreApp assets along anymore which bounds the number of .NETCoreApp assets in a package.