Improve performance and reduce allocations when parsing RedisResults

[shaunsales]

Currently when a RedisResult[] is returned from a TS.RANGE or similar query, we enumerate the entire result and allocate it to a new IReadOnlyCollection. This feels unnecessary, and iteration should be left up to the user.

private static TimeStamp ParseTimeStamp(RedisResult result)
{
	if (result.Type == ResultType.None) return default;
	return new TimeStamp((long)result);
}

private static TimeSeriesTuple ParseTimeSeriesTuple(RedisResult result)
{
	RedisResult[] redisResults = (RedisResult[])result;
	if (redisResults.Length == 0) return null;
	return new TimeSeriesTuple(ParseTimeStamp(redisResults[0]), (double)redisResults[1]);
}

private static IReadOnlyList<TimeSeriesTuple> ParseTimeSeriesTupleArray(RedisResult result)
{
	RedisResult[] redisResults = (RedisResult[])result;
	var list = new List<TimeSeriesTuple>(redisResults.Length);
	if (redisResults.Length == 0) return list;
	Array.ForEach(redisResults, tuple => list.Add(ParseTimeSeriesTuple(tuple)));
	return list;
}

I propose we wrap up the RedisResult into a TsTimeSeriesCollection object, something like;

public class TsTimeSeriesCollection
{
	private RedisResult[] _redisResults;

	public TimeSeriesCollection(RedisResult redisResult)
	{
		_redisResults = (RedisResult[])redisResult;
	}

	public (long TimeStamp, double Value) this[int index] => ((long)((RedisResult[]) _redisResults[index])[0], (double) ((RedisResult[]) _redisResults[index])[1]);

	public int Count => _redisResults.Length;
}

The above will only cast the samples RedisResult when it's accessed, which should help with performance when large sample sets are returned. I haven't extended this to implement IEnumerable<(long,double)> or started optimizing, but the general idea is to create a wrapper around the RedisResult[] and make time series arrays less allocatey and easier to work with.

Since this is a breaking change, it might make sense to do prior to the next release. Feedback welcome!

[DvirDukhan]

@shaunsales great input, as always.

Let's iterate over the design as I would like to have the IEnumerable also implemented. This will cause it to be an implementation of IReadOnlyList
for an example

using System;
using System.Collections;
using System.Collections.Generic;

namespace NRedisTimeSeries.DataTypes
{
    public class TimeSeriesCollection : IReadOnlyList<(long, double)>
    {
        public TimeSeriesCollection()
        {
        }

        public (long, double) this[int index] => throw new NotImplementedException();

        public int Count => throw new NotImplementedException();

        public IEnumerator<(long, double)> GetEnumerator()
        {
            throw new NotImplementedException();
        }

        IEnumerator IEnumerable.GetEnumerator()
        {
            throw new NotImplementedException();
        }
    }
}

What do you think?
Thanks for your inputs

[shaunsales]

Here's a first cut of a timeseries collection class that I think would meet our requirements;

public class TsCollection : IEnumerator<(long TimeStamp, double Value)>
{
	private int _index = -1;

	private RedisResult[] _redisResults;

	public TsCollection(RedisResult redisResult)
	{
		_redisResults = (RedisResult[])redisResult;
	}

	public (long TimeStamp, double Value) this[int index]
	{
		get
		{
			var item = (RedisResult[])_redisResults[index];
			return ((long)item[0], (double)item[1]);
		}
	}
	
	public int Count => _redisResults.Length;

	public IEnumerator<(long TimeStamp, double Value)> GetEnumerator() => this;

	public (long TimeStamp, double Value) Current => (_index > -1 && _index <= _redisResults.Length) ? this[_index] : throw new IndexOutOfRangeException();

	object IEnumerator.Current => Current;

	public bool MoveNext() => ++_index < _redisResults.Length;

	public void Dispose() => Reset();

	public void Reset() => _index = -1;
}

I've not implemented it as a readonly or immutable collection as that adds quite a bit of overhead that I didn't feel was necessary. If there's a good reason to make the collection immutable, I suggest we use ImmutableArray<T> or ImmutableList<T> but given the underlying RedisResult[] is not immutable or readonly it seems better to follow the same pattern.

[DvirDukhan]

@shaunsales
Let's split the logic between the enumerator and the direct access functionality
something like

using System;
using System.Collections;
using System.Collections.Generic;
using StackExchange.Redis;

namespace NRedisTimeSeries.DataTypes
{

    public class TsCollection : IReadOnlyList<(long TimeStamp, double Value)>
    {

        private static (long, double) ResultAsTuple(RedisResult result)
        {
            var item = (RedisResult[])result;
            return ((long)item[0], (double)item[1]);
        }

        private class TsCollectionEnumerator : IEnumerator<(long TimeStamp, double Value)>
        {
            private int _index = -1;
            RedisResult[] _results;
            public TsCollectionEnumerator(TsCollection collection)
            {
                _results = collection._redisResults;
            }

            public (long TimeStamp, double Value) Current => (_index > -1 && _index <= _results.Length) ? ResultAsTuple(_results[_index]) : throw new IndexOutOfRangeException();

            object IEnumerator.Current => Current;

            public bool MoveNext() => ++_index < _results.Length;

            public void Reset() => _index = -1;

            public void Dispose() => Reset();

        }

        private RedisResult[] _redisResults;

        public TsCollection(RedisResult redisResult)
        {
            _redisResults = (RedisResult[])redisResult;
        }

        public (long, double) this[int index]
        {
            get => ResultAsTuple(_redisResults[index]);
 
        }

        public int Count => _redisResults.Length;

        public IEnumerator<(long, double)> GetEnumerator() => new TsCollectionEnumerator(this);

        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
    }
}

WDYT?

[shaunsales]

Looking good. I've made a few (quick and dirty) updates to include MRANGE as a collection of ranges with Name and Label support.

public class TsMRangeResults : IReadOnlyList<TsMRangeResult>
{
	private class TsMRangeEnumerator : IEnumerator<TsMRangeResult>
	{
		private int _index = -1;

		private IReadOnlyList<TsMRangeResult> _tsMRangeResults;

		public TsMRangeEnumerator(TsMRangeResults collection) => _tsMRangeResults = collection._tsMRangeResults;

		public TsMRangeResult Current => (_index > -1 && _index <= _tsMRangeResults.Count) ? _tsMRangeResults[_index] : throw new IndexOutOfRangeException();

		object IEnumerator.Current => Current;

		public bool MoveNext() => ++_index < _tsMRangeResults.Count;

		public void Reset() => _index = -1;

		public void Dispose() => Reset();
	}
	
	private IReadOnlyList<TsMRangeResult> _tsMRangeResults;

	public int Count { get; }

	public TsMRangeResults(RedisResult redisResult)
	{
		var redisResults = (RedisResult[])redisResult;

		Count = redisResults.Length;

		if (redisResults.Length > 0)
		{
			var list = new List<UserQuery.TsMRangeResult>(redisResults.Length);
			for (int i = 0; i < redisResults.Length; i++)
			{
				list.Add(new TsMRangeResult(redisResults[i]));
			}
			_tsMRangeResults = list;
		}
	}

	public TsMRangeResult this[int index] => _tsMRangeResults[index];

	public IEnumerator<TsMRangeResult> GetEnumerator() => new TsMRangeEnumerator(this);

	IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

public class TsMRangeResult : TsRangeResult
{
	public string Name { get; }

	public IList<(string, string)> Labels { get; }

	public TsMRangeResult(RedisResult redisResult) : base((RedisResult[])redisResult)
	{
		var redisResults = (RedisResult[])redisResult;

		Name = (string)redisResults[0];

		var labels = (RedisResult[])redisResults[1];

		if (labels.Length > 0)
		{
			Labels = new List<(string, string)>(labels.Length);

			for (int i = 0; i < labels.Length; i++)
			{
				var labelValue = (RedisResult[])labels[i];
				Labels.Add(((string)labelValue[0], (string)labelValue[1]));
			}
		}
	}
}

public class TsRangeResult : IReadOnlyList<(long TimeStamp, double Value)>
{
	private static (long TimeStamp, double Value) ResultAsTuple(RedisResult result)
	{
		var item = (RedisResult[])result;
		return ((long)item[0], (double)item[1]);
	}

	private class TsRangeResultEnumerator : IEnumerator<(long TimeStamp, double Value)>
	{
		private int _index = -1;

		RedisResult[] _redisResults;

		public TsRangeResultEnumerator(TsRangeResult collection) => _redisResults = collection._redisResults;

		public (long TimeStamp, double Value) Current => (_index > -1 && _index <= _redisResults.Length) ? ResultAsTuple(_redisResults[_index]) : throw new IndexOutOfRangeException();

		object IEnumerator.Current => Current;

		public bool MoveNext() => ++_index < _redisResults.Length;

		public void Reset() => _index = -1;

		public void Dispose() => Reset();
	}

	private RedisResult[] _redisResults;

	public int Count { get; }

	public TsRangeResult(RedisResult redisResult)
	{
		// TODO: Add some data shape checks
		_redisResults = (RedisResult[])redisResult;
		
		Count = _redisResults.Length;
	}

	protected TsRangeResult(RedisResult[] redisResults)
	{
		_redisResults = (RedisResult[])redisResults[2];
	}

	public (long TimeStamp, double Value) this[int index] => ResultAsTuple(_redisResults[index]);

	public IEnumerator<(long TimeStamp, double Value)> GetEnumerator() => new TsRangeResultEnumerator(this);

	IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

This will allow users to access the timeseries results with fairly simply syntax. Here's a couple of examples;

var tsMRangeResults = new TsMRangeResults(db.Execute("TS.MRANGE", new[] { "-", "+", "WITHLABELS", "FILTER", "lbl=abc" }));

foreach (var mRange in tsMRangeResults)
{
	foreach (var item in mRange)
	{
		$"{mRange.Name} {item.TimeStamp}:{item.Value} {mRange.Labels.Count}".Dump();
	}
}

var tsRange = new TsRangeResult(db.Execute("TS.RANGE", new[] { key, "-", "+" }));

foreach (var item in tsRange)
{
	$"{item.TimeStamp}:{item.Value}".Dump();
}

[DvirDukhan]

Nice approach
I think that you can apply the iterator approach everywhere here so you will not have to allocate lists at all
Let's first finish with PR #35 and continue with this

[shaunsales]

Nice approach

I think that you can apply the iterator approach everywhere here so you will not have to allocate lists at all

Let's first finish with PR #35 and continue with this

Agreed - the labels implementation was a bit quick and dirty. I think we can optimise and improve this once we turn it into a PR.