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

perf: faster ament_libraries_deduplicate implementation #448

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

VRichardJP
Copy link
Contributor

@VRichardJP VRichardJP commented May 11, 2023

A faster implementation of ament_libraries_deduplicate().
See #447

Also related: #442

@VRichardJP
Copy link
Contributor Author

@sloretz any update?

@xmfcx
Copy link

xmfcx commented Feb 13, 2024

@VRichardJP could you rebase this branch onto latest humble branch?(27ce75b)

We will use your PR branch within the Autoware project through:

@cottsay
Copy link
Contributor

cottsay commented Feb 28, 2024

Hi @VRichardJP.

Would you mind merging or rebasing to include the latest changes in the rolling branch, as well as updating your sign-off to please the DCO bot? It says:

Expected "Vincent Richard <vincent.francois.richard@gmail.com>", but got "Vincent Richard <richard-v@macnica.co.jp>".

The sign-off needs to match the commit author.

@VRichardJP
Copy link
Contributor Author

@cottsay done!

@cottsay
Copy link
Contributor

cottsay commented Feb 29, 2024

Looks like the tests I added for ament_libraries_deduplicate are failing. It appears this change modifies the behavior of the function under some conditions.

We will need to either re-align the behavior or sufficiently explain why the new behavior is more correct.

Signed-off-by: Vincent Richard <vincent.francois.richard@gmail.com>
@VRichardJP
Copy link
Contributor Author

Nice catch
I think the regex is fixed now.

Before:

$ cmake -P ament_cmake_libraries/test/test_deduplicate.cmake 
CMake Error at ament_cmake_libraries/test/utilities.cmake:6 (message):
  Assert failed: Expected 'debug;foo;debug;baz;debug;bar', got
  'debug;foo;baz;debug;bar'
Call Stack (most recent call first):
  ament_cmake_libraries/test/test_deduplicate.cmake:21 (assert_equal)


CMake Error at ament_cmake_libraries/test/utilities.cmake:6 (message):
  Assert failed: Expected 'debug;foo;debug;bar;debug;baz;bar', got
  'debug;foo;debug;baz;bar'
Call Stack (most recent call first):
  ament_cmake_libraries/test/test_deduplicate.cmake:26 (assert_equal)


CMake Error at ament_cmake_libraries/test/utilities.cmake:6 (message):
  Assert failed: Expected 'debug;foo;debug;bar;debug;baz;release;bar', got
  'debug;foo;debug;baz;release;bar'
Call Stack (most recent call first):
  ament_cmake_libraries/test/test_deduplicate.cmake:31 (assert_equal)

After: no error

VRichardJP added a commit to VRichardJP/ament_cmake that referenced this pull request Mar 1, 2024
see ament#448 (comment)

Signed-off-by: Vincent Richard <richard-v@macnica.co.jp>
@VRichardJP
Copy link
Contributor Author

@cottsay by the way, in your test you use the "release" keyword, but I don't think it is used in the original code

see https://github.com/ament/ament_cmake/pull/448/files#diff-c2367dcaba4bcc1d6bd25ee305a50f29b844d0555aa0f595b57d877d371abe31L33

@cottsay
Copy link
Contributor

cottsay commented Mar 1, 2024

you use the "release" keyword, but I don't think it is used in the original code

Well spotted! Please feel free to change that to one of the supported keywords.

@VRichardJP
Copy link
Contributor Author

@cottsay maybe it make more sense to have this in a different PR, as this one focuses on performance only.

@cottsay
Copy link
Contributor

cottsay commented Mar 1, 2024

maybe it make more sense to have this in a different PR, as this one focuses on performance only

Sure, #516

Signed-off-by: Scott K Logan <logans@cottsay.net>
@cottsay
Copy link
Contributor

cottsay commented Mar 18, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-RHEL Build Status
  • Windows Build Status

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

@sloretz sloretz merged commit 833d2b3 into ament:rolling Mar 29, 2024
3 checks passed
@VRichardJP VRichardJP deleted the faster_deduplicate branch March 30, 2024 00:45
youtalk pushed a commit to youtalk/ament_cmake that referenced this pull request May 16, 2024
Signed-off-by: Vincent Richard <vincent.francois.richard@gmail.com>
Signed-off-by: Scott K Logan <logans@cottsay.net>
Co-authored-by: Scott K Logan <logans@cottsay.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants