-
Notifications
You must be signed in to change notification settings - Fork 43
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
Better Dynamic Fields Solution #107
Conversation
mrut2pac
commented
Apr 1, 2021
- Moved the field parsing methods into a helper class and updated some of the references
- Added all "dynamic" field properties to IUpdateSummaryMessage, marking the DynamicFields property as a candidate for removal
- Added NotImplementedException throwing stubs for the added "dynamic" properties for UpdateSummaryMessage and UpdateSummaryDynamicMessage classes
- 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. :)
- Added unit tests to cover all of the functionality for the generated objects
- Added a new Level1MessageUniversalHandler which utilizes the runtime generated classes and allows changing the dynamic fields without need to recreate the client
- Added an example how to use Level1MessageUniversalHandler
- Added references to System.Reflection.Emit and System.Reflection.Emit.ILGeneration packages in the client project
- Added few methods to StringExtensions to shortcut the IL code generation
…me 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
src/IQFeed.CSharpApiClient.Examples/Examples/StreamingLevel1/StreamingLevel1UniversalExample.cs
Outdated
Show resolved
Hide resolved
@mrut2pac I think we need some kind of benchmark to support this improvement no? |
@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. |
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. |
thats very good and interesting work I have to say. few questions though. (just doing my devil advocate here)
|
Yes sorry, I was checking the code on my phone :) |
using System; | ||
using IQFeed.CSharpApiClient.Streaming.Level1.Messages; | ||
|
||
namespace IQFeed.CSharpApiClient.Streaming.Level1.Handlers |
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.
can we mark the other one deprecated ?
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.
we sure can
@@ -20,6 +20,8 @@ public interface IUpdateSummaryMessage | |||
double Close { get; } | |||
string MessageContents { get; } | |||
string MostRecentTradeConditions { get; } | |||
|
|||
// TODO: drop this for the next MAJOR version, once we migrate to the new solution |
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.
mark it deprecated?
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.
sure, let's decide if we are OK adding the new fields to IUpdateSummaryMessage
or we still want to keep the new interface.
I will make this change
@BunkerCoder what do you think? |
Thanks!
|
@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. |
|
src/IQFeed.CSharpApiClient/Streaming/Level1/Messages/UpdateSummaryMessage.cs
Show resolved
Hide resolved
@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 ? |
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. |
Have you looked at my latest version? There is IUpdateSummaryDynamicMessage interface and generated object now implement it.
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. |
okay lets try that. |
@mrut2pac do you want to wait for the new client or you want to merge this ASAP? |
…me 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
No worries, if you are ready to merge please go ahead. I will fix the conflicts in my branch, if any. |
sorry about it... i wont to it again, or at least without notifying you. |
Okay just merged, please rebase yours, Ill merge it as well. |
done. thanks! |
|
||
public void SelectUpdateFieldName(params DynamicFieldset[] fieldNames) | ||
{ | ||
var dynamicFieldHandler = _level1MessageHandler as ILevel1MessageDynamicHandler; | ||
if (dynamicFieldHandler == null) | ||
throw new Exception("Level1MessageDynamicHandler is required to use DynamicFields property."); | ||
throw new Exception($"{nameof(Level1MessageDynamicHandler)} is required to enable Dynamic Fields!"); |
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.
nice catch!
@@ -3,8 +3,7 @@ | |||
|
|||
namespace IQFeed.CSharpApiClient.Streaming.Level1 | |||
{ | |||
public class BaseLevel1Client | |||
|
|||
public abstract class BaseLevel1Client |
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.
nice! thanks
|
||
namespace IQFeed.CSharpApiClient.Streaming.Level1 | ||
{ | ||
public interface ILevel1EventCommon |
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.
IBaseLevel1Event maybe... I dont know. Its minor. Lets review that later.
Lets merge this baby!
@mrut2pac damn. I wanted to squash these commits... one second. |
please re-create the PR from your branch., |
Nice work guys. Thanks! Quick heads-up. |
Jumped the gun, sorry :) created a new PR |
@BunkerCoder could you please check if you're on the latest? Might have been an intermediate state |
HAHAHAH i'll call you lucky luke... pretty quick on the trigger!!! Merged now!! Thank you so much |
@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. |
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
<#107 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABTDKSVOGAYC53YU634OHGLTHC463ANCNFSM42F4YCWQ>
.
|
@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. |
@mathpaquette - my mistake - those are on my side...I'll resolve. Sorry... |
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:
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...). |