Add test case for slowness in deeply-nested usingRecursiveComparison().isEqualTo() uses in #3350
Closed
ash211 wants to merge 10 commits intoassertj:mainfrom
Closed
Add test case for slowness in deeply-nested usingRecursiveComparison().isEqualTo() uses in #3350ash211 wants to merge 10 commits intoassertj:mainfrom
ash211 wants to merge 10 commits intoassertj:mainfrom
Conversation
…).isEqualTo() uses
Member
|
Thanks for reporting this @ash211, I'll look into it within the next few days. |
Contributor
FieldLocation maintains set of paths to use in rules ordered from leaf to parent. The set provides O(1) hierarchyMatches operation.
Member
|
@schlosna the additions you did look good, do you mind having your version in this PR so that I start integrating it ? |
Contributor
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. |
…wness-repro Optimize FieldLocation
Contributor
Author
|
Merged David’s PR into this one. Thank you! |
Member
Contributor
Author
|
Much appreciated, thank you @joel-costigliola :D |
Contributor
|
Excellent! @joel-costigliola thanks again for the great library and quick turnaround! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
FieldLocation_Test.should_build_from_long_nested_path_in_reasonable_time()<-- PASSRecursiveComparisonConfiguration_getActualNonIgnoreFields_Test.should_return_fields_in_a_reasonable_amount_of_time_for_deeply_nested_object()<-- PASSRecursiveComparisonAssert_isEqualTo_Test.can_compare_deeply_nested_objects_in_reasonable_time()<-- FAILThe 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