[TimeCache] Improve performance for insertData() and pruneList() #680

EricCousineau-TRI · 2024-05-14T01:22:05Z

Resolves #676

Per #676 (comment), this should yield a ~1000x speedup for insertData (for inserting new data only)

EricCousineau-TRI · 2024-05-14T15:05:14Z

Hm. This is breaking test_tf2/buffer_core_test

EDIT: Locally, seems fixed by 898b4b3

Specifically: test_tf2/buffer_core_test/BufferCore_lookupTransform/ring_45_configuration

tf2/src/cache.cpp

EricCousineau-TRI · 2024-05-15T16:29:03Z

@clalancette @ahcorde If y'all have time, would y'all also be able to review + run full CI for this PR?

ahcorde · 2024-05-15T19:30:08Z

Linux
Linux-aarch64
Windows

tf2/src/cache.cpp

Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

ahcorde · 2024-05-16T09:03:52Z

Linux
Linux-aarch64
Windows

clalancette

This looks great; I just added a request for one more comment. Otherwise, this will be ready to get CI run on it.

clalancette · 2024-05-16T12:46:07Z

tf2/src/cache.cpp

@@ -250,23 +250,38 @@ bool TimeCache::insertData(const TransformStorage & new_data)
 {
  const TimePoint latest_time = getLatestTimestamp();

+  // Always prune data. This (a) ensures we trim data we should not access, and
+  // (b) ensures we don't waste time iterating over items to be removed.
+  pruneList();


I like doing this first. It both improves performance for below, and also means that the list is always pruned, even if we are not going to insert the data.

tf2/src/cache.cpp

clalancette · 2024-05-22T17:27:17Z

CI:

Linux
Linux-aarch64
Windows

ahcorde · 2024-05-22T21:05:01Z

https://github.com/Mergifyio backport jazzy

mergify · 2024-05-22T21:05:08Z

backport jazzy

✅ Backports have been created

#686 [TimeCache] Improve performance for insertData() and pruneList() (backport #680) has been created for branch jazzy

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global> Co-authored-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit d700d78)

EricCousineau-TRI · 2024-05-25T19:07:52Z

is it possible to backport this for humble (and maybe iron) as well?

… (#686) Signed-off-by: Eric Cousineau <eric.cousineau@tri.global> Co-authored-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit d700d78) Co-authored-by: Eric Cousineau <eric.cousineau@tri.global>

EricCousineau-TRI · 2024-05-28T14:37:01Z

(We are very interested in having this improvement in humble, so please let me know if there is anything I can do to help in this respect!)

ahcorde · 2024-05-28T16:10:03Z

Ey @EricCousineau-TRI,

I'm backporting some of your last PRs, if CI is green should be done today, then I will create a release

EricCousineau-TRI · 2024-05-28T17:23:00Z

thanks! Just looking though, and I'm not sure if I saw this one (perf improvement) backported for humble.
Did I miss that one?

EricCousineau-TRI · 2024-05-28T17:24:26Z

I ask because I only see jazzy and rolling if I search with this title:
https://github.com/ros2/geometry2/pulls?q=is%3Apr+is%3Aclosed+%5BTimeCache%5D+Improve+performance+for+insertData%28%29+and+pruneList%28%29

ahcorde · 2024-05-28T23:08:37Z

https://github.com/Mergifyio backport humble iron

mergify · 2024-05-28T23:08:47Z

backport humble iron

✅ Backports have been created

#693 [TimeCache] Improve performance for insertData() and pruneList() (backport #680) has been created for branch humble but encountered conflicts
#694 [TimeCache] Improve performance for insertData() and pruneList() (backport #680) has been created for branch iron but encountered conflicts

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global> Co-authored-by: Chris Lalancette <clalancette@gmail.com> (cherry picked from commit d700d78) # Conflicts: # tf2/src/cache.cpp

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global> Co-authored-by: Chris Lalancette <clalancette@gmail.com>

…kport #680) (#694) * Nacho/minor fixes tf2 cache (#658) * Remove unused parameter * Make use of API function to improve redability ```cpp TimePoint TimeCache::getLatestTimestamp() { return storage_.front().stamp_; } ``` And std::list<T>::front() is(gcclib): ```cpp reference front() _GLIBCXX_NOEXCEPT { return *begin(); } ``` * Same argument as 321bd22 ```cpp TimePoint TimeCache::getLatestTimestamp() { // empty list case // ... return storage_.front().stamp_; } ``` and std::list<T>::front(): ```cpp reference front() _GLIBCXX_NOEXCEPT { return *begin(); } ``` * Improve readbility by relying on STL functions By now reading to this block I can tell that we are preventing to inserting a new element in the list, that has a timestamp that is actually older than the max_storage_time_ we allow for * Remove hardcoded algorithmg for STL one The intent of the code is now more clear, instead of relying on raw loops, we "find if" there is any element in the list that has a stamp older than the incoming one. With this we find the position in the list where we should insert the current timestamp: `storage_it` * Remove to better express what this pointer is represetngin * Replace raw loop for STL algorithm Remove if any element is older thant the max_storage_time_ allowed, relative to the latest(sooner) time seems clear npw * [TimeCache] Improve performance for insertData() and pruneList() (#680) Signed-off-by: Eric Cousineau <eric.cousineau@tri.global> Co-authored-by: Chris Lalancette <clalancette@gmail.com> * Don't break ABI Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> --------- Signed-off-by: Eric Cousineau <eric.cousineau@tri.global> Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> Co-authored-by: Ignacio Vizzo <ignacio@dexory.com> Co-authored-by: Eric Cousineau <eric.cousineau@tri.global> Co-authored-by: Chris Lalancette <clalancette@gmail.com> Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

…kport #680) (#693) * Nacho/minor fixes tf2 cache (#658) * Remove unused parameter * Make use of API function to improve redability ```cpp TimePoint TimeCache::getLatestTimestamp() { return storage_.front().stamp_; } ``` And std::list<T>::front() is(gcclib): ```cpp reference front() _GLIBCXX_NOEXCEPT { return *begin(); } ``` * Same argument as 321bd22 ```cpp TimePoint TimeCache::getLatestTimestamp() { // empty list case // ... return storage_.front().stamp_; } ``` and std::list<T>::front(): ```cpp reference front() _GLIBCXX_NOEXCEPT { return *begin(); } ``` * Improve readbility by relying on STL functions By now reading to this block I can tell that we are preventing to inserting a new element in the list, that has a timestamp that is actually older than the max_storage_time_ we allow for * Remove hardcoded algorithmg for STL one The intent of the code is now more clear, instead of relying on raw loops, we "find if" there is any element in the list that has a stamp older than the incoming one. With this we find the position in the list where we should insert the current timestamp: `storage_it` * Remove to better express what this pointer is represetngin * Replace raw loop for STL algorithm Remove if any element is older thant the max_storage_time_ allowed, relative to the latest(sooner) time seems clear npw * [TimeCache] Improve performance for insertData() and pruneList() (#680) Signed-off-by: Eric Cousineau <eric.cousineau@tri.global> Co-authored-by: Chris Lalancette <clalancette@gmail.com> * Don't break ABI Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> --------- Signed-off-by: Eric Cousineau <eric.cousineau@tri.global> Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com> Co-authored-by: Ignacio Vizzo <ignacio@dexory.com> Co-authored-by: Eric Cousineau <eric.cousineau@tri.global> Co-authored-by: Chris Lalancette <clalancette@gmail.com> Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>

[TimeCache] Improve performance for insertData() and pruneList()

8b16415

EricCousineau-TRI requested review from ahcorde and clalancette as code owners May 14, 2024 01:22

This was referenced May 14, 2024

[humble] Optimize "already present" check for TimeCache::insertData() #677

Closed

[DO NOT REVIEW] Testing, benchmark, and fix for TimeCache #681

Closed

Performance regression due to #636 #676

Closed

preserve original insertion order for buffer_core_test

898b4b3

Specifically: test_tf2/buffer_core_test/BufferCore_lookupTransform/ring_45_configuration

EricCousineau-TRI mentioned this pull request May 14, 2024

[cache_unittest] Add direct implementation testing on ordering, pruning #678

Merged

nachovizzo reviewed May 15, 2024

View reviewed changes

tf2/src/cache.cpp Show resolved Hide resolved

clalancette requested changes May 15, 2024

View reviewed changes

tf2/src/cache.cpp Outdated Show resolved Hide resolved

tf2/src/cache.cpp Show resolved Hide resolved

EricCousineau-TRI and others added 3 commits May 15, 2024 16:39

review - fix spelling

07f5ca6

Co-authored-by: Chris Lalancette <clalancette@gmail.com> Signed-off-by: Eric Cousineau <eric.cousineau@tri.global>

Merge remote-tracking branch 'origin/rolling' into issue-676-rolling-fix

ec5cf03

always prune; prune before iterating

215d047

ahcorde approved these changes May 16, 2024

View reviewed changes

clalancette requested changes May 16, 2024

View reviewed changes

update comment overview for implementation

6571fe9

clalancette approved these changes May 22, 2024

View reviewed changes

ahcorde merged commit d700d78 into ros2:rolling May 22, 2024
2 checks passed

mergify bot mentioned this pull request May 22, 2024

[TimeCache] Improve performance for insertData() and pruneList() (backport #680) #686

Merged

mergify bot mentioned this pull request May 28, 2024

[TimeCache] Improve performance for insertData() and pruneList() (backport #680) #693

Merged

mergify bot mentioned this pull request May 28, 2024

[TimeCache] Improve performance for insertData() and pruneList() (backport #680) #694

Merged

ahcorde pushed a commit that referenced this pull request May 29, 2024

[TimeCache] Improve performance for insertData() and pruneList() (#680)

1a721d3

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global> Co-authored-by: Chris Lalancette <clalancette@gmail.com>

ahcorde pushed a commit that referenced this pull request May 29, 2024

[TimeCache] Improve performance for insertData() and pruneList() (#680)

d1143f5

Signed-off-by: Eric Cousineau <eric.cousineau@tri.global> Co-authored-by: Chris Lalancette <clalancette@gmail.com>

This was referenced May 29, 2024

geometry2: 0.31.7-1 in 'iron/distribution.yaml' [bloom] ros/rosdistro#41384

Merged

geometry2: 0.25.7-1 in 'humble/distribution.yaml' [bloom] ros/rosdistro#41385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TimeCache] Improve performance for insertData() and pruneList() #680

[TimeCache] Improve performance for insertData() and pruneList() #680

EricCousineau-TRI commented May 14, 2024 •

edited

Loading

EricCousineau-TRI commented May 14, 2024 •

edited

Loading

EricCousineau-TRI commented May 15, 2024

ahcorde commented May 15, 2024

ahcorde commented May 16, 2024

clalancette left a comment

clalancette May 16, 2024

clalancette commented May 22, 2024

ahcorde commented May 22, 2024

mergify bot commented May 22, 2024 •

edited

Loading

EricCousineau-TRI commented May 25, 2024

EricCousineau-TRI commented May 28, 2024

ahcorde commented May 28, 2024

EricCousineau-TRI commented May 28, 2024

EricCousineau-TRI commented May 28, 2024

ahcorde commented May 28, 2024

mergify bot commented May 28, 2024 •

edited

Loading

[TimeCache] Improve performance for insertData() and pruneList() #680

[TimeCache] Improve performance for insertData() and pruneList() #680

Conversation

EricCousineau-TRI commented May 14, 2024 • edited Loading

EricCousineau-TRI commented May 14, 2024 • edited Loading

EricCousineau-TRI commented May 15, 2024

ahcorde commented May 15, 2024

ahcorde commented May 16, 2024

clalancette left a comment

Choose a reason for hiding this comment

clalancette May 16, 2024

Choose a reason for hiding this comment

clalancette commented May 22, 2024

ahcorde commented May 22, 2024

mergify bot commented May 22, 2024 • edited Loading

✅ Backports have been created

EricCousineau-TRI commented May 25, 2024

EricCousineau-TRI commented May 28, 2024

ahcorde commented May 28, 2024

EricCousineau-TRI commented May 28, 2024

EricCousineau-TRI commented May 28, 2024

ahcorde commented May 28, 2024

mergify bot commented May 28, 2024 • edited Loading

✅ Backports have been created

EricCousineau-TRI commented May 14, 2024 •

edited

Loading

EricCousineau-TRI commented May 14, 2024 •

edited

Loading

mergify bot commented May 22, 2024 •

edited

Loading

mergify bot commented May 28, 2024 •

edited

Loading