Skip to content

Mark VBuffer readonly and convert usages to use 'in' instead of 'ref` #1454

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 31, 2018
Merged

Mark VBuffer readonly and convert usages to use 'in' instead of 'ref` #1454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
60ecb06
Convert readonly VBuffer usages to `in` instead of `ref` parameters.
eerhardt Oct 29, 2018
3e62190
Convert readonly VBuffer usages to `in` instead of `ref` parameters.
eerhardt Oct 29, 2018
3ca8282
Convert readonly VBuffer usages to `in` instead of `ref` parameters.
eerhardt Oct 29, 2018
ae8f11e
Undo in changes due to Roslyn bug
eerhardt Oct 30, 2018
7a096cd
PR feedback.
eerhardt Oct 31, 2018
c1f73d5
Merge upstream/master
eerhardt Oct 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/Microsoft.ML.Core/Data/VBuffer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace Microsoft.ML.Runtime.Data
/// is passed to a row cursor getter, the callee is free to take ownership of
/// and re-use the arrays (Values and Indices).
/// </summary>
public struct VBuffer<T>
public readonly struct VBuffer<T>
{
/// <summary>
/// The logical length of the buffer.
Expand Down Expand Up @@ -424,11 +424,6 @@ public static void Copy(T[] src, int srcIndex, ref VBuffer<T> dst, int length)
dst = new VBuffer<T>(length, values, dst.Indices);
}

public static void Copy(ref VBuffer<T> src, ref VBuffer<T> dst)
Copy link
Member Author

@eerhardt eerhardt Oct 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Note: there was only 1 usage of this method, and it is just a wrapper over src.CopyTo(ref dst), so I thought it was best to drop it in favor of only having a single way to copy from one buffer to another.

Let me know if this was a bad idea. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also used once in internal repos, so I think this is a good change.

In reply to: 229444488 [](ancestors = 229444488)

{
src.CopyTo(ref dst);
}

public IEnumerable<KeyValuePair<int, T>> Items(bool all = false)
{
return VBufferUtils.Items(Values, Indices, Length, Count, all);
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.ML.Core/Prediction/ITree.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public interface ITree<TFeatures> : ITree
/// <param name="features">Type of features container (instance) on which to make predictions</param>
/// <returns>node id</returns>
/// <typeparamref name="TFeatures">Type of features container (instance) on which to make predictions</typeparamref>
int GetLeaf(ref TFeatures features);
int GetLeaf(in TFeatures features);
}

/// <summary>
Expand Down
50 changes: 26 additions & 24 deletions src/Microsoft.ML.Core/Utilities/VBufferUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ internal static IEnumerable<T> DenseValues<T>(T[] values, int[] indices, int len
}
}

public static bool HasNaNs(ref VBuffer<Single> buffer)
public static bool HasNaNs(in VBuffer<Single> buffer)
{
for (int i = 0; i < buffer.Count; i++)
{
Expand All @@ -101,7 +101,7 @@ public static bool HasNaNs(ref VBuffer<Single> buffer)
return false;
}

public static bool HasNaNs(ref VBuffer<Double> buffer)
public static bool HasNaNs(in VBuffer<Double> buffer)
{
for (int i = 0; i < buffer.Count; i++)
{
Expand All @@ -111,7 +111,7 @@ public static bool HasNaNs(ref VBuffer<Double> buffer)
return false;
}

public static bool HasNonFinite(ref VBuffer<Single> buffer)
public static bool HasNonFinite(in VBuffer<Single> buffer)
{
for (int i = 0; i < buffer.Count; i++)
{
Expand All @@ -121,7 +121,7 @@ public static bool HasNonFinite(ref VBuffer<Single> buffer)
return false;
}

public static bool HasNonFinite(ref VBuffer<Double> buffer)
public static bool HasNonFinite(in VBuffer<Double> buffer)
{
for (int i = 0; i < buffer.Count; i++)
{
Expand All @@ -147,7 +147,7 @@ public static VBuffer<T> CreateDense<T>(int length)
/// Applies <paramref name="visitor"/> to every explicitly defined element of the vector,
/// in order of index.
/// </summary>
public static void ForEachDefined<T>(ref VBuffer<T> a, Action<int, T> visitor)
public static void ForEachDefined<T>(in VBuffer<T> a, Action<int, T> visitor)
{
Contracts.CheckValue(visitor, nameof(visitor));

Expand Down Expand Up @@ -175,7 +175,7 @@ public static void ForEachDefined<T>(ref VBuffer<T> a, Action<int, T> visitor)
/// <param name="b">The second vector</param>
/// <param name="visitor">Delegate to apply to each pair of non-zero values.
/// This is passed the index, and two values</param>
public static void ForEachBothDefined<T>(ref VBuffer<T> a, ref VBuffer<T> b, Action<int, T, T> visitor)
public static void ForEachBothDefined<T>(in VBuffer<T> a, in VBuffer<T> b, Action<int, T, T> visitor)
{
Contracts.Check(a.Length == b.Length, "Vectors must have the same dimensionality.");
Contracts.CheckValue(visitor, nameof(visitor));
Expand Down Expand Up @@ -220,7 +220,7 @@ public static void ForEachBothDefined<T>(ref VBuffer<T> a, ref VBuffer<T> b, Act
/// <param name="a">a vector</param>
/// <param name="b">another vector</param>
/// <param name="visitor">Function to apply to each pair of non-zero values - passed the index, and two values</param>
public static void ForEachEitherDefined<T>(ref VBuffer<T> a, ref VBuffer<T> b, Action<int, T, T> visitor)
public static void ForEachEitherDefined<T>(in VBuffer<T> a, in VBuffer<T> b, Action<int, T, T> visitor)
{
Contracts.Check(a.Length == b.Length, "Vectors must have the same dimensionality.");
Contracts.CheckValue(visitor, nameof(visitor));
Expand Down Expand Up @@ -492,7 +492,7 @@ public static void DensifyFirst<T>(ref VBuffer<T> dst, int denseCount)
/// Creates a maybe sparse copy of a VBuffer.
/// Whether the created copy is sparse or not is determined by the proportion of non-default entries compared to the sparsity parameter.
/// </summary>
public static void CreateMaybeSparseCopy<T>(ref VBuffer<T> src, ref VBuffer<T> dst, InPredicate<T> isDefaultPredicate, float sparsityThreshold = SparsityThreshold)
public static void CreateMaybeSparseCopy<T>(in VBuffer<T> src, ref VBuffer<T> dst, InPredicate<T> isDefaultPredicate, float sparsityThreshold = SparsityThreshold)
{
Contracts.CheckParam(0 < sparsityThreshold && sparsityThreshold < 1, nameof(sparsityThreshold));
if (!src.IsDense || src.Length < 20)
Expand Down Expand Up @@ -573,9 +573,9 @@ public static void CreateMaybeSparseCopy<T>(ref VBuffer<T> src, ref VBuffer<T> d
/// <param name="src">Argument vector, whose elements are only read</param>
/// <param name="dst">Argument vector, that could change</param>
/// <param name="manip">Function to apply to each pair of elements</param>
public static void ApplyWith<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer<TDst> dst, PairManipulator<TSrc, TDst> manip)
public static void ApplyWith<TSrc, TDst>(in VBuffer<TSrc> src, ref VBuffer<TDst> dst, PairManipulator<TSrc, TDst> manip)
{
ApplyWithCore(ref src, ref dst, manip, outer: false);
ApplyWithCore(in src, ref dst, manip, outer: false);
}

/// <summary>
Expand All @@ -589,12 +589,13 @@ public static void ApplyWith<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer<TDst
/// there is any slot that is not explicitly represented in either vector.
/// </summary>
/// <param name="src">Argument vector, whose elements are only read</param>
/// <param name="dst">Argument vector, whose elements are only read</param>
/// <param name="dst">Argument vector, whose elements are read in most cases. But in some
/// cases <paramref name="dst"/> may be densified.</param>
/// <param name="res">Result vector</param>
/// <param name="manip">Function to apply to each pair of elements</param>
public static void ApplyWithCopy<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer<TDst> dst, ref VBuffer<TDst> res, PairManipulatorCopy<TSrc, TDst> manip)
public static void ApplyWithCopy<TSrc, TDst>(in VBuffer<TSrc> src, ref VBuffer<TDst> dst, ref VBuffer<TDst> res, PairManipulatorCopy<TSrc, TDst> manip)
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Oct 30, 2018 •
edited by eerhardt
Loading

Choose a reason for hiding this comment

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

ref VBuffer dst [](start = 75, length = 21)

/// Argument vector, whose elements are only read #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I will update this comment, since it is lying.

In reply to: 229498527 [](ancestors = 229498527)

{
ApplyWithCoreCopy(ref src, ref dst, ref res, manip, outer: false);
ApplyWithCoreCopy(in src, ref dst, ref res, manip, outer: false);
}

/// <summary>
Expand All @@ -608,9 +609,9 @@ public static void ApplyWithCopy<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer<
/// <param name="src">Argument vector, whose elements are only read</param>
/// <param name="dst">Argument vector, that could change</param>
/// <param name="manip">Function to apply to each pair of elements</param>
public static void ApplyWithEitherDefined<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer<TDst> dst, PairManipulator<TSrc, TDst> manip)
public static void ApplyWithEitherDefined<TSrc, TDst>(in VBuffer<TSrc> src, ref VBuffer<TDst> dst, PairManipulator<TSrc, TDst> manip)
{
ApplyWithCore(ref src, ref dst, manip, outer: true);
ApplyWithCore(in src, ref dst, manip, outer: true);
}

/// <summary>
Expand All @@ -622,12 +623,13 @@ public static void ApplyWithEitherDefined<TSrc, TDst>(ref VBuffer<TSrc> src, ref
/// there is any slot that is not explicitly represented in either vector.
/// </summary>
/// <param name="src">Argument vector, whose elements are only read</param>
/// <param name="dst">Argument vector, whose elements are only read</param>
/// <param name="dst">Argument vector, whose elements are read in most cases. But in some
/// cases <paramref name="dst"/> may be densified.</param>
/// <param name="res">Result vector</param>
/// <param name="manip">Function to apply to each pair of elements</param>
public static void ApplyWithEitherDefinedCopy<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer<TDst> dst, ref VBuffer<TDst> res, PairManipulatorCopy<TSrc, TDst> manip)
public static void ApplyWithEitherDefinedCopy<TSrc, TDst>(in VBuffer<TSrc> src, ref VBuffer<TDst> dst, ref VBuffer<TDst> res, PairManipulatorCopy<TSrc, TDst> manip)
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Oct 30, 2018 •
edited by eerhardt
Loading

Choose a reason for hiding this comment

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

ref [](start = 88, length = 3)

/// Argument vector, whose elements are only read #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I will update this comment, since it is lying.

In reply to: 229498807 [](ancestors = 229498807)

{
ApplyWithCoreCopy(ref src, ref dst, ref res, manip, outer: true);
ApplyWithCoreCopy(in src, ref dst, ref res, manip, outer: true);
}

/// <summary>
Expand All @@ -636,7 +638,7 @@ public static void ApplyWithEitherDefinedCopy<TSrc, TDst>(ref VBuffer<TSrc> src,
/// where necessary depending on whether this is an inner or outer join of the
/// indices of <paramref name="src"/> on <paramref name="dst"/>.
/// </summary>
private static void ApplyWithCore<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer<TDst> dst, PairManipulator<TSrc, TDst> manip, bool outer)
private static void ApplyWithCore<TSrc, TDst>(in VBuffer<TSrc> src, ref VBuffer<TDst> dst, PairManipulator<TSrc, TDst> manip, bool outer)
{
Contracts.Check(src.Length == dst.Length, "Vectors must have the same dimensionality.");
Contracts.CheckValue(manip, nameof(manip));
Expand Down Expand Up @@ -773,7 +775,7 @@ private static void ApplyWithCore<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer
// This is unnecessary -- falling through to the sparse code will
// actually handle this case just fine -- but it is more efficient.
Densify(ref dst);
ApplyWithCore(ref src, ref dst, manip, outer);
ApplyWithCore(in src, ref dst, manip, outer);
return;
}

Expand Down Expand Up @@ -908,7 +910,7 @@ private static void ApplyWithCore<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer
/// where necessary depending on whether this is an inner or outer join of the
/// indices of <paramref name="src"/> on <paramref name="dst"/>.
/// </summary>
private static void ApplyWithCoreCopy<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer<TDst> dst, ref VBuffer<TDst> res, PairManipulatorCopy<TSrc, TDst> manip, bool outer)
private static void ApplyWithCoreCopy<TSrc, TDst>(in VBuffer<TSrc> src, ref VBuffer<TDst> dst, ref VBuffer<TDst> res, PairManipulatorCopy<TSrc, TDst> manip, bool outer)
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Oct 30, 2018 •
edited by eerhardt
Loading

Choose a reason for hiding this comment

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

ref [](start = 80, length = 3)

can it be "in"? #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

This dst (and the two calling methods above) cannot be made in because of the case at this line of code:

machinelearning/src/Microsoft.ML.Core/Utilities/VBufferUtils.cs

Lines 1089 to 1093 in ef2bade

// result will become dense.
// This is unnecessary -- falling through to the sparse code will
// actually handle this case just fine -- but it is more efficient.
Densify(ref dst);
ApplyWithCoreCopy(ref src, ref dst, ref res, manip, outer);

This method actually modifies dst in that case, by making it dense. Thus, the dst can't be marked in.

In reply to: 229498975 [](ancestors = 229498975)

{
Contracts.Check(src.Length == dst.Length, "Vectors must have the same dimensionality.");
Contracts.CheckValue(manip, nameof(manip));
Expand Down Expand Up @@ -1090,7 +1092,7 @@ private static void ApplyWithCoreCopy<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBu
// This is unnecessary -- falling through to the sparse code will
// actually handle this case just fine -- but it is more efficient.
Densify(ref dst);
ApplyWithCoreCopy(ref src, ref dst, ref res, manip, outer);
ApplyWithCoreCopy(in src, ref dst, ref res, manip, outer);
}
else
{
Expand Down Expand Up @@ -1152,7 +1154,7 @@ private static void ApplyWithCoreCopy<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBu
/// </summary>
/// <seealso cref="ApplyWith{TSrc,TDst}"/>
/// <seealso cref="ApplyWithEitherDefined{TSrc,TDst}"/>
public static void ApplyIntoEitherDefined<TSrc, TDst>(ref VBuffer<TSrc> src, ref VBuffer<TDst> dst, Func<int, TSrc, TDst> func)
public static void ApplyIntoEitherDefined<TSrc, TDst>(in VBuffer<TSrc> src, ref VBuffer<TDst> dst, Func<int, TSrc, TDst> func)
{
Contracts.CheckValue(func, nameof(func));

Expand Down Expand Up @@ -1189,7 +1191,7 @@ public static void ApplyIntoEitherDefined<TSrc, TDst>(ref VBuffer<TSrc> src, ref
/// necessarily be dense. Otherwise, if both are sparse, the output will be sparse iff
/// there is any slot that is not explicitly represented in either vector.
/// </summary>
public static void ApplyInto<TSrc1, TSrc2, TDst>(ref VBuffer<TSrc1> a, ref VBuffer<TSrc2> b, ref VBuffer<TDst> dst, Func<int, TSrc1, TSrc2, TDst> func)
public static void ApplyInto<TSrc1, TSrc2, TDst>(in VBuffer<TSrc1> a, in VBuffer<TSrc2> b, ref VBuffer<TDst> dst, Func<int, TSrc1, TSrc2, TDst> func)
{
Contracts.Check(a.Length == b.Length, "Vectors must have the same dimensionality.");
Contracts.CheckValue(func, nameof(func));
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.ML.Data/Data/BufferBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ public void Reset(int length, bool dense)
SetActiveRangeImpl(0, length);
}

public void AddFeatures(int index, ref VBuffer<T> buffer)
public void AddFeatures(int index, in VBuffer<T> buffer)
{
Contracts.Check(0 <= index && index <= _length - buffer.Length);

Expand Down
8 changes: 4 additions & 4 deletions src/Microsoft.ML.Data/DataLoadSave/Binary/BinarySaver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ public override void FetchAndWrite()
{
Contracts.Assert(_writer != null);
_getter(ref _value);
_writer.Write(ref _value);
_writer.Write(in _value);
}

public override MemoryStream EndBlock()
Expand Down Expand Up @@ -362,7 +362,7 @@ private IValueCodec WriteMetadataCore<T>(Stream stream, ISchema schema, int col,
MemoryStream uncompressedMem = _memPool.Get();
using (IValueWriter<T> writer = codec.OpenWriter(uncompressedMem))
{
writer.Write(ref value);
writer.Write(in value);
writer.Commit();
}
MemoryStream compressedMem = _memPool.Get();
Expand Down Expand Up @@ -791,7 +791,7 @@ private void EstimatorCore<T>(IRowCursor cursor, ColumnCodec col,
fetchWriteEstimator = () =>
{
getter(ref val);
specificWriter.Write(ref val);
specificWriter.Write(in val);
return specificWriter.GetCommitLengthEstimate();
};
}
Expand Down Expand Up @@ -867,7 +867,7 @@ public bool TryWriteTypeAndValue<T>(Stream stream, ColumnType type, ref T value,

using (var writer = codecT.OpenWriter(stream))
{
writer.Write(ref value);
writer.Write(in value);
bytesWritten += (int)writer.GetCommitLengthEstimate();
writer.Commit();
}
Expand Down
16 changes: 8 additions & 8 deletions src/Microsoft.ML.Data/DataLoadSave/Binary/Codecs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ public virtual void Dispose()
}
}

public abstract void Write(ref T value);
public abstract void Write(in T value);

public virtual void Write(T[] values, int index, int count)
{
Contracts.Assert(0 <= index && index <= Utils.Size(values));
Contracts.Assert(0 <= count && count <= Utils.Size(values) - index);
// Basic un-optimized reference implementation.
for (int i = 0; i < count; ++i)
Write(ref values[i + index]);
Write(in values[i + index]);
}

public abstract void Commit();
Expand Down Expand Up @@ -214,7 +214,7 @@ public Writer(UnsafeTypeCodec<T> codec, Stream stream)
_ops = codec._ops;
}

public override void Write(ref T value)
public override void Write(in T value)
{
_ops.Write(value, Writer);
_numWritten++;
Expand Down Expand Up @@ -346,7 +346,7 @@ public Writer(TextCodec codec, Stream stream)
_boundaries = new List<int>();
}

public override void Write(ref ReadOnlyMemory<char> value)
public override void Write(in ReadOnlyMemory<char> value)
{
Contracts.Check(_builder != null, "writer was already committed");
_builder.AppendMemory(value);
Expand Down Expand Up @@ -456,7 +456,7 @@ public Writer(BoolCodec codec, Stream stream)
{
}

public override void Write(ref bool value)
public override void Write(in bool value)
{
Contracts.Assert(0 <= _currentIndex && _currentIndex < 8);

Expand Down Expand Up @@ -620,7 +620,7 @@ public Writer(DateTimeCodec codec, Stream stream)
{
}

public override void Write(ref DateTime value)
public override void Write(in DateTime value)
{
Writer.Write(value.Ticks);
_numWritten++;
Expand Down Expand Up @@ -698,7 +698,7 @@ public Writer(DateTimeOffsetCodec codec, Stream stream)
_ticks = new List<long>();
}

public override void Write(ref DateTimeOffset value)
public override void Write(in DateTimeOffset value)
{
Contracts.Assert(_offsets != null, "writer was already committed");

Expand Down Expand Up @@ -927,7 +927,7 @@ public override long GetCommitLengthEstimate()
return structureLength + _valueWriter.GetCommitLengthEstimate();
}

public override void Write(ref VBuffer<T> value)
public override void Write(in VBuffer<T> value)
{
Contracts.Check(_valuesStream != null, "writer already committed");
if (FixedLength)
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.ML.Data/DataLoadSave/Binary/IValueCodec.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ internal interface IValueWriter<T> : IValueWriter
/// <summary>
/// Writes a single value to the writer.
/// </summary>
void Write(ref T value);
void Write(in T value);

/// <summary>
/// Writes an array of values. This should be equivalent to writing each element
Expand Down
Loading