Skip to content

Conversation

@ivanpauno
Copy link
Collaborator

@ivanpauno ivanpauno commented Jan 20, 2022

This PR applies two minor improvements:

  • (1) Avoid to repeating some iterations over the same colections.
    Doesn't reduce time complexity, but it reduces the total amount of "operations" done.
  • (2) Cache the created wait set and resize it.

The second item seems to be a good improvement as allocation/deallocation is slow.
I have still some tests issues that I'm trying to figure out. Solved!

I have done some profiling, though nothing great (I need to create a demo with more entities to get more interesting results)
To profile:

  • Download visualvm, it can be run using ./visualvm_212/bin/visualvm after extracting the archive.
  • Run the listener and publisher demos:
    • ros2 run rcljava_examples subscriber_lambda &
    • ros2 run rcljava_examples publisher_lambda &
  • Profile the subscriber_lambda
    • Pick the org.ros2.rcljava.examples.subscriber.SubscriberLambda (pid <pid>) entry in Local.
    • Go to the profiler tab and add org.ros2.rcljava.executors.** to the "profile classes:" list.
    • Start running "CPU" profiling.

I have compared between this branch and main:

  • In my machine, (1) only improved performance marginally (could've been measurement error).
    It's reasonable because I'm only testing with one subscription available.
  • (2) gives a more noticeable improvement.
    waitForWork() went from taking 50% of the time to 37%.
    We still need to test in a more realistic application to confirm.

Note: To make the measurements stable, I have disabled frequently scaling in my machine temporally.
See for example here for details.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno ivanpauno added the enhancement New feature or request label Jan 20, 2022
@ivanpauno ivanpauno requested a review from jacobperron January 20, 2022 20:43
@ivanpauno ivanpauno self-assigned this Jan 20, 2022
Signed-off-by: Ivan Santiago Paunovic <[email protected]>
@ivanpauno
Copy link
Collaborator Author

I have still some tests issues that I'm trying to figure out.

I knew I was doing something wrong there when I saw the test failures, but I couldn't find the problem 😂.
Fixed it in 80ff53d.

Copy link

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'd like to test this out before we merge it.

Signed-off-by: Ivan Santiago Paunovic <[email protected]>

Co-authored-by: Jacob Perron <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants