mathpaquette / IQFeed.CSharpApiClient

IQFeed.CSharpApiClient is fastest and the most well-designed C# DTN IQFeed socket API connector available
MIT License
120 stars 43 forks source link

Dynamic Fieldsets - Work in Progress #69

Closed BunkerCoder closed 4 years ago

BunkerCoder commented 4 years ago

Seeking feedback on the design. Sorry for the random changes to non-code files...I'm new to Github. Search for "CURRENT UPDATE FIELDNAMES" and the design ideas should flow from there.

mathpaquette commented 4 years ago

are you planning to make the build pass? I like the idea the support this feature. It makes total sense but I dont want to impact the performance of the for those who are not using it in most cases.

lets explore to have a Level1ClientDynamic which supports it and by reusing most of the code that already exists.

BunkerCoder commented 4 years ago

Need to noodle on your recent posts above. This message was in-the-works... You've given me a lot to think about. Sorry about the "change your mind" thing - that was rude. Level1ClientDynamic using dynamics would make for a super-elegant solution, and that would be in keeping with your excellent design work. I have to be honest that I had put dynamics on my "use only in case of emergency" list when I learned about the severe performance hit (Good read: https://stackoverflow.com/questions/7478387/how-does-having-a-dynamic-variable-affect-performance). For my current project, they are definitely a no-go. The latest commit of "DynamicFieldset_Bunker" branch in my fork builds. It works properly for all of the examples, but only supports one additional field (Open Interest) at this point. I also cleaned up a few uglies. I went with the strongly-typed approach for performance, but, after further reflection, I can see that taking my design to it's logical conclusion would add over 70 new properties to the L1 message, and even that's too much bloat for my taste (but I'll probably use it in my local branch in the short term to meet my immediate need for a few select fields). I'm a little concerned about cross-contamination between Level1ClientDynamic and Level1Client (similar to Issue #44), as apps using Level1Client would break if a Level1ClientDynamic was also used because they share the same feed fields. How about something like this: a new L1 Property that returns an array of abstract DynamicField objects. Each feed field will need to be a subclass. Constructor takes feed message string. Value property performs the parsing of the message string (only about 5 versions needed) with lazy initialization. User code would look something like this: var OpenInterest = L1Msg.DynamicFields[i]).Value or use a (cast) to grab the field itself: var myOpenInterestField = (IQFeed.CSharpApiClient.Streaming.Level1.DynamicFields.OpenInterest) L1Msg.DynamicFields[i] With this approach, current Level1Client code would not break, nor take a performance hit (except for the little extra to init the empty DynamicField array).

mathpaquette commented 4 years ago

@BunkerCoder okay I agree that dynamics might not be the long term solution and by the same time we're claiming that we have the greatest level of performance out there.

I like the idea of having a new message class like this one:

class Program
    {
        static void Main(string[] args)
        {
            var updateMessage = new UpdateSummaryMessage(new[] { DynamicFieldset.Bid, DynamicFieldset.BidSize }, new List<object> { 10.5, 1000 });
            var bid = updateMessage.Bid;
            var bidSize = updateMessage.BidSize;
        }
    }

    class UpdateSummaryMessage
    {
        private readonly Dictionary<DynamicFieldset, object> _valuesByFieldType;

        public UpdateSummaryMessage(IList<DynamicFieldset> fields, IList<object> values)
        {
            _valuesByFieldType = new Dictionary<DynamicFieldset, object>(fields.Select((x, i) => new KeyValuePair<DynamicFieldset, object>(x, values[i])));
        }

        public double Bid
        {
            get
            {
                if (_valuesByFieldType.TryGetValue(DynamicFieldset.Bid, out var value))
                {
                    return (double)value;
                }

                throw new Exception("not found");
            }
        }

        public int BidSize
        {
            get
            {
                if (_valuesByFieldType.TryGetValue(DynamicFieldset.BidSize, out var value))
                {
                    return (int)value;
                }

                throw new Exception("not found");
            }
        }
    }

    internal class FieldsetDescriptionAttribute : Attribute
    {
        public string Name { get; }
        public Type Type { get; }

        public FieldsetDescriptionAttribute(string name, Type type)
        {
            Name = name;
            Type = type;
        }
    }

    public enum DynamicFieldset
    {
        [FieldsetDescription("Bid", typeof(double))]
        Bid,

        [FieldsetDescription("BidSize", typeof(int))]
        BidSize,
    }
mathpaquette commented 4 years ago

@BunkerCoder any thoughts?

BunkerCoder commented 4 years ago

@mathpaquette - sorry for dropping out; had some life events crop up (still bubbling). Your new UpdateSummaryMessage looks great. I'm still doing some analysis to minimize code changes and maximize reuse of Level1Client, possibly without creating Level1ClientDynamic. Current thinking is to add new CreateNew() overloads in Level1ClientFactory that add DynamicFieldset[] parameter (or I could modify existing and make nullable). Possibly add public event Action SummaryDynamic and public event Action UpdateDynamic. I found 6 parsing variations. Considering the following:

public enum DynamicFieldParseType
{
    ParseChar,
    ParseDateTimeDateOnly,
    ParseDateTimeTimeOnly,
    ParseDouble,
    ParseInt,
    ParseString
}

internal class FieldsetDescriptionAttribute : Attribute
{
    public string Name { get; }
    public Type Type { get; }
    public DynamicFieldParseType ParseType { get; }

    public FieldsetDescriptionAttribute(string name, Type type, DynamicFieldParseType parseType)
    {
        Name = name;
        Type = type;
        ParseType = parseType;
    }
}

    [FieldsetDescription("Ask Time", typeof(DateTime), DynamicFieldParseType.ParseDateTimeTimeOnly)]
    AskTime,

    [FieldsetDescription("Financial Status Indicator", typeof(char), DynamicFieldParseType.ParseChar)]
    FinancialStatusIndicator,
...

Hope to have something for you to review within a few days, but maybe a little longer. I'm new to tests (as well as GitHub and open source). Haven't spent much time looking at your tests yet. That will probably teach me much of what I need to know, but if you can recommend any reading that will help with etiquette, I'd appreciate it. I'm considering dropping the DynamicFieldsets_Bunker branch and going with a new branch. Not really sure of etiquette in that regard either, so any tips appreciated. Thanks for your May 31 commits; standardizing on double will make life much simpler.

mathpaquette commented 4 years ago

no problem take all the time you need. I would also prefer to have everything inside Level1Client if you can manage that without hitting too much the performance. TBH I did some perf tests like adding 80 properties on the UpdateSummaryMessage. Not a too big hit but still.

BunkerCoder commented 4 years ago

Abandoned this branch. Will create a new PR.