-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Convert ValueMapper to use 'in' parameters #1475
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,13 +9,13 @@ namespace Microsoft.ML.Runtime.Data | |
/// <summary> | ||
/// Delegate type to map/convert a value. | ||
/// </summary> | ||
public delegate void ValueMapper<TSrc, TDst>(ref TSrc src, ref TDst dst); | ||
public delegate void ValueMapper<TSrc, TDst>(in TSrc src, ref TDst dst); | ||
|
||
/// <summary> | ||
/// Delegate type to map/convert among three values, for example, one input with two | ||
/// outputs, or two inputs with one output. | ||
/// </summary> | ||
public delegate void ValueMapper<TVal1, TVal2, TVal3>(ref TVal1 val1, ref TVal2 val2, ref TVal3 val3); | ||
public delegate void ValueMapper<TVal1, TVal2, TVal3>(in TVal1 val1, ref TVal2 val2, ref TVal3 val3); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I inspected all the usages of this delegate, and most times it is used by the below interface I didn't feel it was necessary to split this delegate. If someone does feel it is necessary, I think a separate issue should be logged and addressed separately. |
||
|
||
/// <summary> | ||
/// Interface for mapping a single input value (of an indicated ColumnType) to | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -116,28 +116,28 @@ protected ValueWriterBase(PrimitiveType type, int source, char sep) | |||||||||||||||||||||||||||||||
Conv = Conversions.Instance.GetStringConversion<T>(type); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
var d = default(T); | ||||||||||||||||||||||||||||||||
Conv(ref d, ref Sb); | ||||||||||||||||||||||||||||||||
Conv(in d, ref Sb); | ||||||||||||||||||||||||||||||||
Default = Sb.ToString(); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
protected void MapText(ref ReadOnlyMemory<char> src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
protected void MapText(in ReadOnlyMemory<char> src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
TextSaverUtils.MapText(src.Span, ref sb, Sep); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
protected void MapTimeSpan(ref TimeSpan src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
protected void MapTimeSpan(in TimeSpan src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a note: some of these don't seem worthwhile to pass by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we see though we benefit from having a uniform signature no matter what the input type is, even if not all types will benefit, since we can then use the same high level code for all types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These have to be changed to machinelearning/src/Microsoft.ML.Data/DataLoadSave/Text/TextSaver.cs Lines 100 to 114 in a8bc268
|
||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
TextSaverUtils.MapTimeSpan(ref src, ref sb); | ||||||||||||||||||||||||||||||||
TextSaverUtils.MapTimeSpan(in src, ref sb); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
protected void MapDateTime(ref DateTime src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
protected void MapDateTime(in DateTime src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
TextSaverUtils.MapDateTime(ref src, ref sb); | ||||||||||||||||||||||||||||||||
TextSaverUtils.MapDateTime(in src, ref sb); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
protected void MapDateTimeZone(ref DateTimeOffset src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
protected void MapDateTimeZone(in DateTimeOffset src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
TextSaverUtils.MapDateTimeZone(ref src, ref sb); | ||||||||||||||||||||||||||||||||
TextSaverUtils.MapDateTimeZone(in src, ref sb); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
|
@@ -170,15 +170,15 @@ public override void WriteData(Action<StringBuilder, int> appendItem, out int le | |||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
for (int i = 0; i < _src.Length; i++) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
Conv(ref _src.Values[i], ref Sb); | ||||||||||||||||||||||||||||||||
Conv(in _src.Values[i], ref Sb); | ||||||||||||||||||||||||||||||||
appendItem(Sb, i); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
for (int i = 0; i < _src.Count; i++) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
Conv(ref _src.Values[i], ref Sb); | ||||||||||||||||||||||||||||||||
Conv(in _src.Values[i], ref Sb); | ||||||||||||||||||||||||||||||||
appendItem(Sb, _src.Indices[i]); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
@@ -195,7 +195,7 @@ public override void WriteHeader(Action<StringBuilder, int> appendItem, out int | |||||||||||||||||||||||||||||||
var name = _slotNames.Values[i]; | ||||||||||||||||||||||||||||||||
if (name.IsEmpty) | ||||||||||||||||||||||||||||||||
continue; | ||||||||||||||||||||||||||||||||
MapText(ref name, ref Sb); | ||||||||||||||||||||||||||||||||
MapText(in name, ref Sb); | ||||||||||||||||||||||||||||||||
int index = _slotNames.IsDense ? i : _slotNames.Indices[i]; | ||||||||||||||||||||||||||||||||
appendItem(Sb, index); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
@@ -218,15 +218,15 @@ public ValueWriter(IRowCursor cursor, PrimitiveType type, int source, char sep) | |||||||||||||||||||||||||||||||
public override void WriteData(Action<StringBuilder, int> appendItem, out int length) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
_getSrc(ref _src); | ||||||||||||||||||||||||||||||||
Conv(ref _src, ref Sb); | ||||||||||||||||||||||||||||||||
Conv(in _src, ref Sb); | ||||||||||||||||||||||||||||||||
appendItem(Sb, 0); | ||||||||||||||||||||||||||||||||
length = 1; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
public override void WriteHeader(Action<StringBuilder, int> appendItem, out int length) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
var span = _columnName.AsMemory(); | ||||||||||||||||||||||||||||||||
MapText(ref span, ref Sb); | ||||||||||||||||||||||||||||||||
MapText(in span, ref Sb); | ||||||||||||||||||||||||||||||||
appendItem(Sb, 0); | ||||||||||||||||||||||||||||||||
length = 1; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
@@ -846,7 +846,7 @@ internal static void MapText(ReadOnlySpan<char> span, ref StringBuilder sb, char | |||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
internal static void MapTimeSpan(ref TimeSpan src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
internal static void MapTimeSpan(in TimeSpan src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
if (sb == null) | ||||||||||||||||||||||||||||||||
sb = new StringBuilder(); | ||||||||||||||||||||||||||||||||
|
@@ -856,7 +856,7 @@ internal static void MapTimeSpan(ref TimeSpan src, ref StringBuilder sb) | |||||||||||||||||||||||||||||||
sb.AppendFormat("\"{0:c}\"", src); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
internal static void MapDateTime(ref DateTime src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
internal static void MapDateTime(in DateTime src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
if (sb == null) | ||||||||||||||||||||||||||||||||
sb = new StringBuilder(); | ||||||||||||||||||||||||||||||||
|
@@ -866,7 +866,7 @@ internal static void MapDateTime(ref DateTime src, ref StringBuilder sb) | |||||||||||||||||||||||||||||||
sb.AppendFormat("\"{0:o}\"", src); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
internal static void MapDateTimeZone(ref DateTimeOffset src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
internal static void MapDateTimeZone(in DateTimeOffset src, ref StringBuilder sb) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
if (sb == null) | ||||||||||||||||||||||||||||||||
sb = new StringBuilder(); | ||||||||||||||||||||||||||||||||
|
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.
Just to confirm, is
TSrc
expected to always be areadonly struct
?in
causes hidden copies when calling an instance member for non-readonly structs and can cause unexpected perf-regressions in some cases.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.
As a practical matter in most applications they will be some sort of
VBuffer
, with some isolated instances here and there as you've observed. Yet these other types from what I see tend to also be readonly structs. (E.g., you observed elsewhereTimeSpan
, and here isInt32
) I just ran a pretty simple benchmark like so (using BenchmarkDotNet, my new favorite project)...At least on my computer this gives:
So it seems like it is at least not harmful for things like
int
, while still giving some benefit for things likeVBuffer
.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.
In my limited knowledge of the area, I would guess that
>95%
of the time ValueMapper will be used for primitives, reference types, andreadonly struct
.The most common case is to use these types:
Along with
VBuffer
, which just got madereadonly
in #1454.Skimming this list, I found that
UInt128
was not markedreadonly
. So I'm opening #1496 to make that (and other obvious readonly structs) marked asreadonly
.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.
Is there any concern with the primitive types not being
readonly structs
for full framework and/or netstandard right now?