Skip to content
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

DYN1805 Validate the performance tracking system by using validated improvements Part II #9665

Merged
merged 12 commits into from
Apr 23, 2019

Conversation

reddyashish
Copy link
Contributor

@reddyashish reddyashish commented Apr 17, 2019

Purpose

This PR is a copy of the one that Craig made: #9620

Instead of removing the TraceUtils class from from Protocore.Lang namespace, we just obsolete it for now.

Since this trace data is checked for in the Execute function, this part of the code is executed for all the graphs. A small percentage decrease or increase in the run times for those graphs, that take less absolute run times can be ignored.

Performance results comparison with the base line data(for model tests):
result1
result2

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner @aparajit-pratap @QilongTang @saintentropy

@QilongTang
Copy link
Contributor

Since this trace data is checked for in the Execute function, this part of the code is executed for all the graphs.

Can you explain a little bit more or point to the actual caller to help understand?

@QilongTang
Copy link
Contributor

QilongTang commented Apr 23, 2019

LGTM @reddyashish Is there anything we are still waiting in this PR?

@QilongTang QilongTang added the LGTM Looks good to me label Apr 23, 2019
@reddyashish
Copy link
Contributor Author

No Aaron, this is good to go. Merging it.

@reddyashish reddyashish merged commit b859c7b into DynamoDS:master Apr 23, 2019
reddyashish added a commit that referenced this pull request May 23, 2019
* Fixing the API break.

* Removing spaces

* Removing spaces

* Removing spaces

* Using the traceUtils.cs class from DynamoServices.

* Spacing
mjkkirschner pushed a commit that referenced this pull request Aug 3, 2019
…mprovements Part II (#9665)

* Move Engine TraceUtils methods to DynamoServices with ThreadStatic backing

* Convert GetTrace and SetTrace to user ThreadStatic mechanism

* Remove Engine TraceUtils

* Clean up tests to remove thread local storage reference

* Clean up callsite Trace workflow

* Remove expression body syntax

* Remove inline out declaration

* Obseleting the class instead of removing it

* Sort usings.

* Removing the TODO comment that is not valid anymore
mjkkirschner pushed a commit that referenced this pull request Aug 3, 2019
* Fixing the API break.

* Removing spaces

* Removing spaces

* Removing spaces

* Using the traceUtils.cs class from DynamoServices.

* Spacing
@junmendoza
Copy link
Contributor

junmendoza commented Sep 6, 2019

Very impressive performance uplift! @aparajit-pratap I'm no longer up to date with the VM but is this improvement attributed to fixing a fundamental issue in the callsites or an iterative process?

@aparajit-pratap
Copy link
Contributor

aparajit-pratap commented Sep 9, 2019

@junmendoza glad to know you are still tracking progress of Dynamo :)
This perf improvement was a result of work by @saintentropy : #9620 to replace the backing implementation of TLS from a thread data slot to a thread relative static field per Microsoft recommendations: https://docs.microsoft.com/en-us/dotnet/standard/threading/thread-local-storage-thread-relative-static-fields-and-data-slots
Since the TLS is checked for each VM interaction in the callsites it basically affects performance globally, not only for trace/element binding related calls.

@junmendoza
Copy link
Contributor

@aparajit-pratap, @saintentropy Interesting! and thanks for the background. Btw did this arise due to performance profiling? Curious how much effort is still put in making the VM faster at this stage :)

@aparajit-pratap
Copy link
Contributor

@saintentropy has been slowly chipping away at the VM trying to make performance improvements and he has made some along the way. We have more ongoing work in this area. One of the things I was looking at is at optimizing execution for modified inputs in graphs: #9485. This was based on one of your design proposals some time ago.
This work is yet to be fully completed and merged.

@reddyashish reddyashish deleted the DYN1805 branch November 23, 2022 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants