Skip to content

WIP: Introduce ReadOnlyVBuffer and use it all over. #1270

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

Closed
wants to merge 1 commit into from

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Oct 15, 2018

This is a prototype of one direction we could take VBuffer - introduce a ReadOnlyVBuffer for methods that are guaranteed not to modify the buffer. That way a caller can safely be assured that the buffer will not be mutated during the method.

Working towards #608.

@eerhardt eerhardt changed the title Introduce ReadOnlyVBuffer and use it all over. WIP: Introduce ReadOnlyVBuffer and use it all over. Oct 15, 2018
@eerhardt eerhardt requested review from Zruty0 and TomFinley October 15, 2018 23:12
@eerhardt
Copy link
Member Author

@Zruty0 @TomFinley - any initial thoughts to this direction? It's not fully complete. I was considering changing the Predictors ValueMapper to take in a ReadOnlyVBuffer<float> instead of a VBuffer<float> , since the Predictors shouldn't be modifying the input buffer.

Let me know what you think, if I should continue this direction or not.

/// <summary>
/// The values. Only the first Count of these are valid.
/// </summary>
public ReadOnlySpan<T> Values => _values;
Copy link
Contributor

Choose a reason for hiding this comment

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

Values => _values [](start = 31, length = 17)

I think this is a good beginning. I feel like though if we redesign VBuffer, my primary goal would be to get rid of all the difficulties that made my writing this document necessary.

That is, I think much of the stuff here should actually be in VBuffer itself.

In particular, I would move this to VBuffer and change this to Values => _values.AsSpan(0, count), and Indices => IsDense ? default : _indices.AsSpan(0, count).

We would need some separate object (I suspect internal) that is capable of mutating a VBuffer, to mutate a VBuffer.

In this world there would still I think be benefit to having a read-only version of VBuffer, since that thing could be a ref struct with direct fields of ReadOnlySpan.

Extenally, both VBuffer and ReadOnlyVuffer would both appear to be immutable objects, but VBuffer's buffers would be internally reusable. ROVB though would be more efficient, since in that.

@Zruty0
Copy link
Contributor

Zruty0 commented Oct 29, 2018

Should we close this now @eerhardt ?

@eerhardt
Copy link
Member Author

Oh yes, sorry. This fell off my radar. Closing as this is no longer the plan.

@eerhardt eerhardt closed this Oct 29, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants