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

Optimize CGrpcServer getters #1284

Merged
merged 13 commits into from
Nov 24, 2023
Merged

Optimize CGrpcServer getters #1284

merged 13 commits into from
Nov 24, 2023

Conversation

PProfizi
Copy link
Contributor

@PProfizi PProfizi commented Nov 21, 2023

Edit: the actual issue was found to be too many gRPC calls to ResultInfo getters when using a LegacyGrpcServer. The solution found was to add a cache decorator to functions called repeatedly.
We thus went down from 1004 gRPC calls waiting a total of 104s, to 166 gRPC calls waiting a total of 15s.

By looking at the GrpcServer calls, caching the os and version properties also decreases total time from 50s to 15s.

There may be further improvements to be made that I did not see with the simple print(model) command.

NB: I did enforce a limit of 50 for the cache while testing, this is TBD
NB2: the first initiative consisting of improving the description given by the server could still be beneficial.
NB3: all changes related to gate have been made in a different PR server-side

Old:
Summary:

Upon arrival of Fluid results, the ResultInfo.__str__ method was enhanced to also show the locations of each available result as they are dynamic, as well as create a map of qualifier names vs IDs, as well as properly detect the fluid physics type.
This creates a performance deficit of 1200% when in LegacyGrpc (without any actual network delay) due to the number of requests made just when requesting for available results.
The goal of this PR is to revert to the server description of the ResultInfo when in LegacyGrpc to bypass this performance bottleneck, while server-side changes are to be made to:

  • add result location to available result descriptions
  • add a map of available qualifiers
  • correctly show the physics type when fluid
  • switch to the server description of the ResultInfo in all cases, potentially with some very minor formatting

@PProfizi PProfizi added the bug Something isn't working label Nov 21, 2023
@PProfizi PProfizi requested a review from cbellot000 November 21, 2023 09:07
@PProfizi PProfizi self-assigned this Nov 21, 2023
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Merging #1284 (9538b91) into master (fced584) will decrease coverage by 1.28%.
Report is 7 commits behind head on master.
The diff coverage is 89.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1284      +/-   ##
==========================================
- Coverage   87.45%   86.18%   -1.28%     
==========================================
  Files          81       81              
  Lines        9234     9248      +14     
==========================================
- Hits         8076     7970     -106     
- Misses       1158     1278     +120     

@PProfizi PProfizi requested a review from rlagha November 22, 2023 10:58
@PProfizi PProfizi changed the title Temporary fix for performance of ResultInfo.__str__() in LegacyGrpc Cache ResultInfo get requests in LegacyGrpc Nov 22, 2023
@PProfizi PProfizi requested a review from a team as a code owner November 22, 2023 11:16
@PProfizi PProfizi changed the title Cache ResultInfo get requests in LegacyGrpc Improve performance for remote Nov 22, 2023
src/ansys/dpf/gate/result_info_grpcapi.py Outdated Show resolved Hide resolved
src/ansys/dpf/gate/result_info_grpcapi.py Outdated Show resolved Hide resolved
src/ansys/dpf/gate/result_info_grpcapi.py Outdated Show resolved Hide resolved
src/ansys/dpf/gate/result_info_grpcapi.py Outdated Show resolved Hide resolved
src/ansys/dpf/gate/result_info_grpcapi.py Outdated Show resolved Hide resolved
@PProfizi PProfizi requested a review from cbellot000 November 23, 2023 15:14
@PProfizi PProfizi changed the title Improve performance for remote Optimize CGrpcServer getters Nov 23, 2023
@PProfizi PProfizi merged commit 8048e2b into master Nov 24, 2023
35 of 36 checks passed
@PProfizi PProfizi deleted the bug/result_info_str_perf_remote branch November 24, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants