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

Add test case for slowness in deeply-nested usingRecursiveComparison().isEqualTo() uses in #3350

Closed

Conversation

ash211
Copy link
Contributor

@ash211 ash211 commented Jan 26, 2024

First, thank you for the assertj project! It's been a joy to work with and we use it extensively across my company's Java ecosystem.

I'm opening this PR to demonstrate a performance regression we've observed between assertj 3.24.2 and 3.25.2. The regression is significant enough that we're unable to take 3.25.2 and are holding back on 3.24.2 for the time being. This is inconvenient though, so we'd like to work together to identify and implement a fix that restores the previous performance characteristics.

There are 3 new test cases:

  1. FieldLocation_Test.should_build_from_long_nested_path_in_reasonable_time() <-- PASS
  2. RecursiveComparisonConfiguration_getActualNonIgnoreFields_Test.should_return_fields_in_a_reasonable_amount_of_time_for_deeply_nested_object() <-- PASS
  3. RecursiveComparisonAssert_isEqualTo_Test.can_compare_deeply_nested_objects_in_reasonable_time() <-- FAIL

The first two test cases pass locally for me, as they execute in a reasonable amount of time. But the third one fails because it takes too long. When copying this test case onto the old 3.24.2 release, it passes. This is the shape of assertion that is breaking for us.

In the new 3.25.2 release, this test case seems to be triggering quadratic behavior in time. Uncommenting each additional link in the list doubles the duration of the test. On my machine the test as written takes 15sec, when I would expect it to be under a second. For reference, on 3.24.2 it takes ~275ms.

I think it was introduced by 0eb44cc but have not confirmed.

cc @joel-costigliola

cc @schlosna we thought 3.25.2 would fix this perf regression -- #3320 (comment) -- but it sadly doesn't

@joel-costigliola
Copy link
Member

Thanks for reporting this @ash211, I'll look into it within the next few days.

@scordio scordio added the theme: recursive comparison An issue related to the recursive comparison label Jan 26, 2024
@schlosna
Copy link
Contributor

@ash211 I created ash211#1 on top of your branch & this PR with some improvements that pass the tests you added and seem to bring things back in line with 3.24.2 performance of recursive comparisons.

@joel-costigliola
Copy link
Member

@schlosna the additions you did look good, do you mind having your version in this PR so that I start integrating it ?

@schlosna
Copy link
Contributor

@schlosna the additions you did look good, do you mind having your version in this PR so that I start integrating it ?

Sounds good to me. If we want it in this PR I think @ash211 has to merge ash211#1 into his branch. I could also open a separate PR directly to assertj that includes both @ash211 test cases and the fix. I'm happy to do either.

@ash211
Copy link
Contributor Author

ash211 commented Jan 28, 2024

Merged David’s PR into this one. Thank you!

@joel-costigliola
Copy link
Member

Integrated thanks for the good work @ash211 and @schlosna!

@ash211
Copy link
Contributor Author

ash211 commented Jan 29, 2024

Much appreciated, thank you @joel-costigliola :D

@ash211 ash211 deleted the aash/usingRecursiveComparison-slowness-repro branch January 29, 2024 18:02
@schlosna
Copy link
Contributor

Integrated thanks for the good work @ash211 and @schlosna!

Excellent! @joel-costigliola thanks again for the great library and quick turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: recursive comparison An issue related to the recursive comparison
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants