usdot-fhwa-stol / cdasim

CDASim is an open-source simulation system supporting the development and testing of Cooperative Driving Automation applications.

38 stars 13 forks source link

CDAR-732: Remove redundant requests #195

Closed paulbourelly999 closed 7 months ago

paulbourelly999 commented 8 months ago

PR Details

Description

Remove redundant requests to XMLRPC CARLA CDA Sim adapter server. Also removed redundant calls to requestNextTime

Related GitHub Issue

Related Jira Key

CDDAR-732

Motivation and Context

Improve performance and clean up logs

How Has This Been Tested?

Tested in CDASim deployment

Types of changes

[x] Defect fix (non-breaking change that fixes an issue)
[ ] New feature (non-breaking change that adds functionality)
[ ] Breaking change (fix or feature that cause existing functionality to change)

Checklist:

[ ] I have added any new packages to the sonar-scanner.properties file
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

kjrush commented 7 months ago

Seems good to me, I'm okay with approving it just on a review, but want to confirm the testing. How long were the test cases you ran? I think we were seeing the previous livelock issue occurring at like 5ish minutes into simulation execution. To my recollection it was stochastic though, it just happened that about 5 minutes was enough timesteps that it was pretty probable to occur.

If we can confirm that's not a problem, I'm down to approve and merge.

paulbourelly999 commented 7 months ago

Seems good to me, I'm okay with approving it just on a review, but want to confirm the testing. How long were the test cases you ran? I think we were seeing the previous livelock issue occurring at like 5ish minutes into simulation execution. To my recollection it was stochastic though, it just happened that about 5 minutes was enough timesteps that it was pretty probable to occur.

If we can confirm that's not a problem, I'm down to approve and merge.

Hey @kjrush out test case is significantly shorted than 5 minutes. Regardless if this bug is there are not after 5 minutes, this is definitely not the fix and seems to at least partially negatively impact performance. I will try to run a 10 minute simulation and see what happens

sonarcloud[bot] commented 7 months ago

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

paulbourelly999 commented 7 months ago

Seems good to me, I'm okay with approving it just on a review, but want to confirm the testing. How long were the test cases you ran? I think we were seeing the previous livelock issue occurring at like 5ish minutes into simulation execution. To my recollection it was stochastic though, it just happened that about 5 minutes was enough timesteps that it was pretty probable to occur. If we can confirm that's not a problem, I'm down to approve and merge.

Hey @kjrush out test case is significantly shorted than 5 minutes. Regardless if this bug is there are not after 5 minutes, this is definitely not the fix and seems to at least partially negatively impact performance. I will try to run a 10 minute simulation and see what happens

@kjrush Tested for 5 minute simulation, other services crashed but no CDASim

paulbourelly999 commented 7 months ago

I am going to merge this and independently see I the issues with RTI event priority dead lock can be recreated. It is possible this change re introduces this behavior but with VRU UC simulation scenarios this behavior seems to not be present. Might be more likely with the addition of SUMO actors. The fix that is undone by this PR is likely not the long term fix. If this PR re introduces the bug we can make a decision independently about whether to find a more long term fix or revert to this fix