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

Some behavioral / perf fixes for Embedding<T> #567

Merged
merged 1 commit into from Apr 26, 2023

Conversation

stephentoub
Copy link
Member

Motivation and Context

Clean up some Embedding<T> behaviors / perf issues.

Description

  • IsSupported was more expensive than it needs to be: if it's changed to just compare the types directly rather than searching a list, the JIT can turn the entire operation into a JIT-time constant.
  • The explicit cast to ReadOnlySpan<T> was cloning the array. It doesn't need to do that.
  • default(Embedding<T>) results in a struct that fails on a bunch of operations with NullReferenceExceptions if you try to use it. We've learned from experience with ImmutableArray<T> this is suboptimal. I've made it functional and behave equivalent to Empty... in fact, Empty is now just default.

Contribution Checklist

@lemillermicrosoft lemillermicrosoft added PR: ready for review All feedback addressed, ready for reviews kernel.core labels Apr 21, 2023
@MovGP0
Copy link

MovGP0 commented Apr 21, 2023

I don't like code in the form of private readonly TEmbedding[]? _vector; as seen in Embeddings.cs.

Enumerable objects should return an empty enumerable object like Array.Empty<T> when not initialized instead of the null value.

@stephentoub
Copy link
Member Author

Enumerable objects should return an empty enumerable object like Array.Empty when not initialized instead of the null value.

Where is it returning null?

@github-actions github-actions bot added the .NET Issue or Pull requests regarding .NET code label Apr 24, 2023
- IsSupported was more expensive than it needs to be: if it's changed to just compare the types directly rather than searching a list, the JIT can turn the entire operation into a JIT-time constant.
- The explicit cast to `ReadOnlySpan<T>` was cloning the array. It doesn't need to do that.
- `default(Embedding<T>)` results in a struct that fails on a bunch of operations with NullReferenceExceptions if you try to use it.  We've learned from experience with `ImmutableArray<T>` this is suboptimal.  I've made it functional and behave equivalent to Empty... in fact, Empty is now just `default`.
@shawncal shawncal enabled auto-merge (squash) April 26, 2023 06:38
@shawncal shawncal merged commit 15dc234 into microsoft:main Apr 26, 2023
11 checks passed
dluc pushed a commit that referenced this pull request Apr 29, 2023
### Motivation and Context

Clean up some `Embedding<T>` behaviors / perf issues.

### Description

- IsSupported was more expensive than it needs to be: if it's changed to
just compare the types directly rather than searching a list, the JIT
can turn the entire operation into a JIT-time constant.
- The explicit cast to `ReadOnlySpan<T>` was cloning the array. It
doesn't need to do that.
- `default(Embedding<T>)` results in a struct that fails on a bunch of
operations with NullReferenceExceptions if you try to use it. We've
learned from experience with `ImmutableArray<T>` this is suboptimal.
I've made it functional and behave equivalent to Empty... in fact, Empty
is now just `default`.
dehoward pushed a commit to lemillermicrosoft/semantic-kernel that referenced this pull request Jun 1, 2023
### Motivation and Context

Clean up some `Embedding<T>` behaviors / perf issues.

### Description

- IsSupported was more expensive than it needs to be: if it's changed to
just compare the types directly rather than searching a list, the JIT
can turn the entire operation into a JIT-time constant.
- The explicit cast to `ReadOnlySpan<T>` was cloning the array. It
doesn't need to do that.
- `default(Embedding<T>)` results in a struct that fails on a bunch of
operations with NullReferenceExceptions if you try to use it. We've
learned from experience with `ImmutableArray<T>` this is suboptimal.
I've made it functional and behave equivalent to Empty... in fact, Empty
is now just `default`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.NET Issue or Pull requests regarding .NET code PR: ready for review All feedback addressed, ready for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants