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

Better Dynamic Fields Solution #107

Closed mrut2pac closed 3 years ago

mrut2pac commented 3 years ago
  1. Moved the field parsing methods into a helper class and updated some of the references
  2. Added all "dynamic" field properties to IUpdateSummaryMessage, marking the DynamicFields property as a candidate for removal
  3. Added NotImplementedException throwing stubs for the added "dynamic" properties for UpdateSummaryMessage and UpdateSummaryDynamicMessage classes
  4. Added new static class that allows generating UpdateSummaryMessage implementations classes on the fly for the selected dynamic fields. Used advanced IL code generation from Reflection.Emit. Tried my best to add enough comments to make the code reading less of a nightmare. :)
  5. Added unit tests to cover all of the functionality for the generated objects
  6. Added a new Level1MessageUniversalHandler which utilizes the runtime generated classes and allows changing the dynamic fields without need to recreate the client
  7. Added an example how to use Level1MessageUniversalHandler
  8. Added references to System.Reflection.Emit and System.Reflection.Emit.ILGeneration packages in the client project
  9. Added few methods to StringExtensions to shortcut the IL code generation
mathpaquette commented 3 years ago

@mrut2pac I think we need some kind of benchmark to support this improvement no?

mathpaquette commented 3 years ago

@mrut2pac I was not expecting to change the default non dynamic interface (IUpdateSummaryMessage) at least for now. Is there a way that we can merge your PR without impacting this one and just when we use DynamicFields ? Just wondering,

mrut2pac commented 3 years ago

@mrut2pac I was not expecting to change the default non dynamic interface (IUpdateSummaryMessage) at least for now. Is there a way that we can merge your PR without impacting this one and just when we use DynamicFields ? Just wondering,

My changes were 100% backwards compatible, except for the case if someone has a logic of listing the IUpdateSummaryMessage fields using reflection. Using DynamicFields won't work cause it is a class and not interface. I added a new interface that adds the dynamic fields on top of IUpdateSummaryMessage and for now the caller has to cast the received message to access the new fields. Hope this will suffice.

mrut2pac commented 3 years ago

@mrut2pac I think we need some kind of benchmark to support this improvement no?

For memory it is obvious that the new solution is better since it creates objects with less number of fields (except when you select all fields, then it will be the same). In terms of CPU usage, I ensured that the generated IL code for each concrete class is practically the same set of instructions (only negligible differences that only affect the compiled IL code size), since there are no loops and branching compared to the current solution where we have to make a check for every field to see if it is included or not (or if the value is different than the default), it "SHOULD" be faster or at least achieve same speed. I agree that benchmarking would have been nice, but unfortunately I don't have more time to spend on this. Already went above my initial estimate. :)

mathpaquette commented 3 years ago

@mrut2pac I think we need some kind of benchmark to support this improvement no?

For memory it is obvious that the new solution is better since it creates objects with less number of fields (except when you select all fields, then it will be the same). In terms of CPU usage, I ensured that the generated IL code for each concrete class is practically the same set of instructions (only negligible differences that only affect the compiled IL code size), since there are no loops and branching compared to the current solution where we have to make a check for every field to see if it is included or not (or if the value is different than the default), it "SHOULD" be faster or at least achieve same speed. I agree that benchmarking would have been nice, but unfortunately I don't have more time to spend on this. Already went above my initial estimate. :)

thats very good and interesting work I have to say. few questions though. (just doing my devil advocate here)

mathpaquette commented 3 years ago

@mrut2pac I was not expecting to change the default non dynamic interface (IUpdateSummaryMessage) at least for now. Is there a way that we can merge your PR without impacting this one and just when we use DynamicFields ? Just wondering,

My changes were 100% backwards compatible, except for the case if someone has a logic of listing the IUpdateSummaryMessage fields using reflection. Using DynamicFields won't work cause it is a class and not interface. I added a new interface that adds the dynamic fields on top of IUpdateSummaryMessage and for now the caller has to cast the received message to access the new fields. Hope this will suffice.

Yes sorry, I was checking the code on my phone :)

mathpaquette commented 3 years ago

@BunkerCoder what do you think?

mrut2pac commented 3 years ago

@mrut2pac I think we need some kind of benchmark to support this improvement no?

For memory it is obvious that the new solution is better since it creates objects with less number of fields (except when you select all fields, then it will be the same). In terms of CPU usage, I ensured that the generated IL code for each concrete class is practically the same set of instructions (only negligible differences that only affect the compiled IL code size), since there are no loops and branching compared to the current solution where we have to make a check for every field to see if it is included or not (or if the value is different than the default), it "SHOULD" be faster or at least achieve same speed. I agree that benchmarking would have been nice, but unfortunately I don't have more time to spend on this. Already went above my initial estimate. :)

thats very good and interesting work I have to say. few questions though. (just doing my devil advocate here)

  • there's a slight performance hit carrying out unused properties for UpdateSummaryMessage, observed it using Level1MessageHandlerPerformanceTests after and before your changes. Just wondering if it would make sense to whether create a new Level1Client like Level1ClientDynamic able to deal with UpdateSummaryMessage with this new set of properties OR make Level1Client generic like we've done float, double, decimal before. Personally, from a user perspective, I think its more convenient the first option but what are your thoughts on this.
  • checking ILevel1Event, do you think having Summary and Update using IUpdateSummaryMessage instead of the class still relevant?
  • do you think there's some duplication between StreamingLevel1DynamicExample and StreamingLevel1UniversalExample can we combine both? Like keeping both seems to be a little bit overkill. Not sure I like the term Universal...

Thanks!

mathpaquette commented 3 years ago

@mrut2pac check Level1MessageHandlerPerformanceTests class and run it on master and your branch. It takes slightly more time on yours probably because of the additional properties on UpdateSummaryMessage.

I'm not talking about the Dynamic Fields at all. Seems that even Level1MessageHandler is impacted because the set of new fields.

mathpaquette commented 3 years ago
mathpaquette commented 3 years ago

@mrut2pac I think we are very close to merge this. Can we get on the same page for the next small refactor about passing dynamic fields in the constructor ?

I was thinking to have a Level1DynamicClient that supports your new Handler and instead of creating all these new properties in existing UpdateSummaryMessage we could have a new UpdateSummaryDynamicMessage and this one will be available in ILevel1Event as a generic for your new Handler. What do you think? That way, there's a clear separation. Personally, I dont use dynamic fields so, I'd prefer to have the leanest possible path.

Any thoughts ?

mrut2pac commented 3 years ago

Level1MessageHandlerPerformanceTests

The current version doesn't have any new fields and I'm getting my branch run slower than master for Debug but faster with Release. Since there is nothing changed for this test at the moment I am inclined to ignore the slight variations as statistically insignificant. :) I will look to write similar test for the current dynamic and new solutions, would be interesting to compare. *** Update: Actually I just realized the parsing code is changed. Interestingly the Release build runs faster... Are you sure you want to keep the code duplication across the board and not use FieldParser? I think we should use the FieldParser...

mrut2pac commented 3 years ago

@mrut2pac I think we are very close to merge this. Can we get on the same page for the next small refactor about passing dynamic fields in the constructor ?

I was thinking to have a Level1DynamicClient that supports your new Handler and instead of creating all these new properties in existing UpdateSummaryMessage we could have a new UpdateSummaryDynamicMessage and this one will be available in ILevel1Event as a generic for your new Handler. What do you think? That way, there's a clear separation. Personally, I dont use dynamic fields so, I'd prefer to have the leanest possible path.

Any thoughts ?

Have you looked at my latest version? There is IUpdateSummaryDynamicMessage interface and generated object now implement it.

Personally, I dont use dynamic fields so, I'd prefer to have the leanest possible path.

Are you using every single field from IUpdateSummaryMessage? If you don't you can still benefit by omitting the unneeded ones...

I will find time and create a new client to work with the new handler.

mathpaquette commented 3 years ago

Level1MessageHandlerPerformanceTests

The current version doesn't have any new fields and I'm getting my branch run slower than master for Debug but faster with Release. Since there is nothing changed for this test at the moment I am inclined to ignore the slight variations as statistically insignificant. :) I will look to write similar test for the current dynamic and new solutions, would be interesting to compare. *** Update: Actually I just realized the parsing code is changed. Interestingly the Release build runs faster... Are you sure you want to keep the code duplication across the board and not use FieldParser? I think we should use the FieldParser...

okay lets try that.

mathpaquette commented 3 years ago

@mrut2pac do you want to wait for the new client or you want to merge this ASAP?

mathpaquette commented 3 years ago

@mrut2pac do you want to wait for the new client or you want to merge this ASAP?

@mrut2pac are we ready to merge this one?

mrut2pac commented 3 years ago

I am still working on it. I wrote the new client and separated the common code. Still got to fo few things. Please wait.

Regards, Vardan

On Apr 3, 2021, at 7:26 AM, Mathieu Paquette @.***> wrote:

 @mrut2pac do you want to wait for the new client or you want to merge this ASAP?

@mrut2pac are we ready to merge this one?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

mathpaquette commented 3 years ago

I am still working on it. I wrote the new client and separated the common code. Still got to fo few things. Please wait. Regards, Vardan On Apr 3, 2021, at 7:26 AM, Mathieu Paquette @.***> wrote:  @mrut2pac do you want to wait for the new client or you want to merge this ASAP? @mrut2pac are we ready to merge this one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

@mrut2pac Ill merge protocol 6.1 PR first. @NichUK proposed that for a while and now almost ready. We might have some minor conflicts afterward.

mrut2pac commented 3 years ago

@mathpaquette finalized the solution.

  1. Refactored to completely separate this new Dynamic flow. Refactored some of the existing classes to minimize the code duplication. When we remove the old dynamic code we can clean it up even more.
  2. Added performance tests to cover both old and new dynamic handler.
  3. The new code generated is much faster than the old dynamic code and is even slightly faster than UpdateSummaryMessage parse code when the standard set of fields are selected. The difference is much more drastic in Debug build. For Release the compiler helps to optimize it, but still my code beats it. Here are some stats: https://docs.google.com/spreadsheets/d/1DkrHW9Doyc_gs2xz0Q90CsulqmUEeshAgrXZlv23_Dw/edit?usp=sharing

Please let me know what you think.

P.S. a kind ask - next time please let me do the reverse merge from master to my branch as it will reduce the thrash and iterations if I do it at the right time ;)

mrut2pac commented 3 years ago

I am still working on it. I wrote the new client and separated the common code. Still got to fo few things. Please wait. Regards, Vardan On Apr 3, 2021, at 7:26 AM, Mathieu Paquette @.***> wrote:  @mrut2pac do you want to wait for the new client or you want to merge this ASAP? @mrut2pac are we ready to merge this one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

@mrut2pac Ill merge protocol 6.1 PR first. @NichUK proposed that for a while and now almost ready. We might have some minor conflicts afterward.

No worries, if you are ready to merge please go ahead. I will fix the conflicts in my branch, if any.

mathpaquette commented 3 years ago

@mathpaquette finalized the solution.

  1. Refactored to completely separate this new Dynamic flow. Refactored some of the existing classes to minimize the code duplication. When we remove the old dynamic code we can clean it up even more.
  2. Added performance tests to cover both old and new dynamic handler.
  3. The new code generated is much faster than the old dynamic code and is even slightly faster than UpdateSummaryMessage parse code when the standard set of fields are selected. The difference is much more drastic in Debug build. For Release the compiler helps to optimize it, but still my code beats it. Here are some stats: https://docs.google.com/spreadsheets/d/1DkrHW9Doyc_gs2xz0Q90CsulqmUEeshAgrXZlv23_Dw/edit?usp=sharing

Please let me know what you think.

P.S. a kind ask - next time please let me do the reverse merge from master to my branch as it will reduce the thrash and iterations if I do it at the right time ;)

sorry about it... i wont to it again, or at least without notifying you.

mathpaquette commented 3 years ago

I am still working on it. I wrote the new client and separated the common code. Still got to fo few things. Please wait. Regards, Vardan On Apr 3, 2021, at 7:26 AM, Mathieu Paquette @.***> wrote:  @mrut2pac do you want to wait for the new client or you want to merge this ASAP? @mrut2pac are we ready to merge this one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

@mrut2pac Ill merge protocol 6.1 PR first. @NichUK proposed that for a while and now almost ready. We might have some minor conflicts afterward.

No worries, if you are ready to merge please go ahead. I will fix the conflicts in my branch, if any.

Okay just merged, please rebase yours, Ill merge it as well.

mrut2pac commented 3 years ago

I am still working on it. I wrote the new client and separated the common code. Still got to fo few things. Please wait. Regards, Vardan On Apr 3, 2021, at 7:26 AM, Mathieu Paquette @.***> wrote:  @mrut2pac do you want to wait for the new client or you want to merge this ASAP? @mrut2pac are we ready to merge this one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

@mrut2pac Ill merge protocol 6.1 PR first. @NichUK proposed that for a while and now almost ready. We might have some minor conflicts afterward.

No worries, if you are ready to merge please go ahead. I will fix the conflicts in my branch, if any.

Okay just merged, please rebase yours, Ill merge it as well.

done. thanks!

mathpaquette commented 3 years ago

@mrut2pac damn. I wanted to squash these commits... one second.

mathpaquette commented 3 years ago

@mrut2pac damn. I wanted to squash these commits... one second.

please re-create the PR from your branch.,

BunkerCoder commented 3 years ago

Nice work guys. Thanks! Quick heads-up. I've been working on getting my test environment up-to-date so that I can do some testing on my live code. Just updated to VS 16.9.3, and started getting missing NumberStyles and CultureInfo errors in Level1DynamicFields.cs. Adding "using System.Globalization;" fixed it. I haven't tracked down the root cause (probably something I goofed), but wanted to pass it along.

mrut2pac commented 3 years ago

@mrut2pac damn. I wanted to squash these commits... one second.

Jumped the gun, sorry :) created a new PR

mrut2pac commented 3 years ago

Nice work guys. Thanks! Quick heads-up. I've been working on getting my test environment up-to-date so that I can do some testing on my live code. Just updated to VS 16.9.3, and started getting missing NumberStyles and CultureInfo errors in Level1DynamicFields.cs. Adding "using System.Globalization;" fixed it. I haven't tracked down the root cause (probably something I goofed), but wanted to pass it along.

@BunkerCoder could you please check if you're on the latest? Might have been an intermediate state

mathpaquette commented 3 years ago

@mrut2pac damn. I wanted to squash these commits... one second.

Jumped the gun, sorry :) created a new PR

HAHAHAH i'll call you lucky luke... pretty quick on the trigger!!! Merged now!! Thank you so much

BunkerCoder commented 3 years ago

Nice work guys. Thanks! Quick heads-up. I've been working on getting my test environment up-to-date so that I can do some testing on my live code. Just updated to VS 16.9.3, and started getting missing NumberStyles and CultureInfo errors in Level1DynamicFields.cs. Adding "using System.Globalization;" fixed it. I haven't tracked down the root cause (probably something I goofed), but wanted to pass it along.

@BunkerCoder could you please check if you're on the latest? Might have been an intermediate state

@mathpaquette - Per https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes, it's the latest. Released 3/30/2021.

mathpaquette commented 3 years ago

@mrut2pac @NichUK quite a productive weekend.. Protocol 6.1 and now this live Dynamic client!! Congrats.

mathpaquette commented 3 years ago

Nice work guys. Thanks! Quick heads-up. I've been working on getting my test environment up-to-date so that I can do some testing on my live code. Just updated to VS 16.9.3, and started getting missing NumberStyles and CultureInfo errors in Level1DynamicFields.cs. Adding "using System.Globalization;" fixed it. I haven't tracked down the root cause (probably something I goofed), but wanted to pass it along.

@BunkerCoder could you please check if you're on the latest? Might have been an intermediate state

@mathpaquette - Per https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes, it's the latest. Released 3/30/2021.

Probably a bad intermediate state .... Make sure you Clean solution and Build.. Usually it helps. We dont merge anything without a successfully CI build in order to avoid such frustrating situation that the build doesnt pass :)

mrut2pac commented 3 years ago

Thanks guys and Happy Easter, if you're celebrating!

On Sun, Apr 4, 2021 at 12:30 PM Mathieu Paquette @.***> wrote:

@mrut2pac https://github.com/mrut2pac @NichUK https://github.com/NichUK quite a productive weekend.. Protocol 6.1 and now this live Dynamic client!! Congrats.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mathpaquette/IQFeed.CSharpApiClient/pull/107#issuecomment-813087559, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABTDKSVOGAYC53YU634OHGLTHC463ANCNFSM42F4YCWQ .

BunkerCoder commented 3 years ago

Nice work guys. Thanks! Quick heads-up. I've been working on getting my test environment up-to-date so that I can do some testing on my live code. Just updated to VS 16.9.3, and started getting missing NumberStyles and CultureInfo errors in Level1DynamicFields.cs. Adding "using System.Globalization;" fixed it. I haven't tracked down the root cause (probably something I goofed), but wanted to pass it along.

@BunkerCoder could you please check if you're on the latest? Might have been an intermediate state

@mathpaquette - Per https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes, it's the latest. Released 3/30/2021.

Probably a bad intermediate state .... Make sure you Clean solution and Build.. Usually it helps. We dont merge anything without a successfully CI build in order to avoid such frustrating situation that the build doesnt pass :)

@mathpaquette - I just pulled latest master (73d15c0), cleaned solution and rebuilt. Now I'm seeing "Merge conflict marker encountered" in Level1Client.cs, Level1DynamicFields.cs, and iUpdateSummaryMessage.cs. Looks legit, but probably my issue again, Checking.

BunkerCoder commented 3 years ago

@mathpaquette - my mistake - those are on my side...I'll resolve. Sorry...

mathpaquette commented 3 years ago

Nice work guys. Thanks! Quick heads-up. I've been working on getting my test environment up-to-date so that I can do some testing on my live code. Just updated to VS 16.9.3, and started getting missing NumberStyles and CultureInfo errors in Level1DynamicFields.cs. Adding "using System.Globalization;" fixed it. I haven't tracked down the root cause (probably something I goofed), but wanted to pass it along.

@BunkerCoder could you please check if you're on the latest? Might have been an intermediate state

@mathpaquette - Per https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes, it's the latest. Released 3/30/2021.

Probably a bad intermediate state .... Make sure you Clean solution and Build.. Usually it helps. We dont merge anything without a successfully CI build in order to avoid such frustrating situation that the build doesnt pass :)

@mathpaquette - I just pulled latest master (73d15c0), cleaned solution and rebuilt. Now I'm seeing "Merge conflict marker encountered" in Level1Client.cs, Level1DynamicFields.cs, and iUpdateSummaryMessage.cs. Looks legit, but probably my issue again, Checking.

Sorry earlier I force pushed on master to come back in a previous state. If you dont have any changes on master (technically should be the case) do this:

git fetch origin
git checkout master
git reset origin/master --hard

you will come back to HEAD

BunkerCoder commented 3 years ago

Nice work guys. Thanks! Quick heads-up. I've been working on getting my test environment up-to-date so that I can do some testing on my live code. Just updated to VS 16.9.3, and started getting missing NumberStyles and CultureInfo errors in Level1DynamicFields.cs. Adding "using System.Globalization;" fixed it. I haven't tracked down the root cause (probably something I goofed), but wanted to pass it along.

@BunkerCoder could you please check if you're on the latest? Might have been an intermediate state

@mathpaquette - Per https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes, it's the latest. Released 3/30/2021.

Probably a bad intermediate state .... Make sure you Clean solution and Build.. Usually it helps. We dont merge anything without a successfully CI build in order to avoid such frustrating situation that the build doesnt pass :)

@mathpaquette - I just pulled latest master (73d15c0), cleaned solution and rebuilt. Now I'm seeing "Merge conflict marker encountered" in Level1Client.cs, Level1DynamicFields.cs, and iUpdateSummaryMessage.cs. Looks legit, but probably my issue again, Checking.

Sorry earlier I force pushed on master to come back in a previous state. If you dont have any changes on master (technically should be the case) do this:

git fetch origin
git checkout master
git reset origin/master --hard

you will come back to HEAD

I should have seen that. Back to HEAD now. Clean/Rebuild All worked. Now I'm frozen trying to open Test Explorer...probably an issue in 16.9.3. I'll figure it out and let you know what happened.

Update - It's Windows... Reboot fixed it... Ran tests locally without failures (but we knew that because of CI...). Thanks again.