DYN-6295 : multithreaded analytics engine separated from model instance #14595
Conversation
|
@zeusongit @sm6srw can you guys please review this as I think it touches the same apis you are touching. |
|
@BogdanZavu Please look at the regressions first |
|
Given that all track calls are now on the thread pool, maybe we need to look more closely at the Analytics.Net project to figure out if all the data is thread safe. I know there are bunch of static properties in there...but I think they are only written to during Initialization. |
|
I believe we are good with Analytics.NET - the initialization part remains single threaded and the few statics there are read-only.
|
|
Yes, some overlap with my obsolete PR but nothing that can't be solved I think. |
|
@QilongTang and team any other concerns ? |
|
|
||
| lock (trackEventLockObj) | ||
| { | ||
| if (!ReportingAnalytics) return Disposable; |
There was a problem hiding this comment.
I think we should try to minimize the amount of code executed within these locks.
Specifically I think we can move the if (!ReportingAnalytics) return Disposable; check above the lock (probably the same for all the locks)
There was a problem hiding this comment.
While ReportingAnalytics does only a null check on the service side right now that could change in the future, I think it doesn't really matter , it's safer to leave it as is.
| @@ -218,7 +218,7 @@ public string Version | |||
| /// <summary> | |||
| /// Host analytics info | |||
| /// </summary> | |||
| public HostAnalyticsInfo HostAnalyticsInfo { get; set; } | |||
| public static HostAnalyticsInfo HostAnalyticsInfo { get; set; } | |||
There was a problem hiding this comment.
This is a good idea and I have some other change would benefit from this 👍🏻
There was a problem hiding this comment.
There are still some places where HostAnalytics is not static (ex. SplashScreen.xaml.cs, PackageManagerExtension.cs, IStartConfiguration and DefaultStartConfiguration, ReadyParams, StartDynamoWithDefaultConfig)
Do they all still make sense to exist and to be non-static ?
Some of these changes will conflict with the "clean obsolete APIs" PR
FYI @sm6srw @mjkkirschner
There was a problem hiding this comment.
Yeah, raised this concern as well above -thanks for pointing out how many non static use cases still exist.
In addition
@zeusongit's PR touches this as well dealing with the fact that Application.Current is no longer null in Revit.
There was a problem hiding this comment.
Imo the start configurations data structures should remain as is. Those are transient and host info from there ends up being set in DynamoModel on startup.
ReadyParams hostinfo ( perhaps we should just remove it ?! ) delegates towards DynamoModel static now.
Handling host info in SplashScreen.xaml.cs and PackageManagerExtension.cs is a little bit above the scope of this PR so let's handle that separately in the other or a different pr. The model not being always available I believe is the reason why we have separate instances of host info in splash screen and packagemanager.
zeusongit
changed the title
|
I feel some future improvements maybe still involved but I would like to merge for now to get things rolling. How and when to set static properties/ singleton is still a challenge in the new Dynamo architecture I can forsee.. |
Purpose
Remove Analytics engine dependency on DynamoModel and perform all analytics operations on worker threads.
This will give us a boos in performance of about 5-7% in startup time and avoid any slowness in ADP systems overall in all usage contexts.
For consumers like Dynamo Player and Generative Design is even more valuable because there is no need to instantiate a Dynamo model anymore and worry about the proper execution thread ( along with other changes Dynamo Player and GD will start instantly ).
There are some API braking changes but they are small and can be mediated in a different way if really needed (some are noted to be obsolete in 3.0 ).
DynamoModel.HostAnalyticsInfo
DynamoModel.Version
DynamoModel.AppVersion
The above Dynamo utilities either don't belong there or can be made static. For the moment I chose to make them static but we can do something else.
Some analytics classes and method have become internal because we want to be able to start/have the analytics engine available when DynamoModel is not started yet. Once DynamoModel starts it will pick up the existing analytics instance.
Related to that sometimes we do not want to shutdown the analytics engine when DynamoModel shuts down - in this case it will be the host application responsibility to shut down the analytics service.
No issues observed so far with the new approach.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@DynamoDS/dynamo