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

feat: add memory metrics to acceptance tests #1874

Merged
merged 35 commits into from
Oct 16, 2024
Merged

Conversation

davidgamez
Copy link
Member

@davidgamez davidgamez commented Oct 7, 2024

Description

Memory consumption depends on the JVM state when the memory is collected; there is a considerable difference between reference(master) and latest(PR) executions.

From our AI friend ;-) :

This pull request includes significant changes to the output-comparator module, focusing on refactoring the memory usage comparison logic and improving the performance metrics collection. The most important changes include the removal of the BoundedPriorityQueue class, the introduction of new comparators, and the refactoring of the ValidationPerformanceCollector class to streamline memory usage reporting.

Memory Usage Comparison Refactor:

  • Removed the BoundedPriorityQueue class, which was previously used for maintaining bounded priority queues of dataset memory usage. (BoundedPriorityQueue.java)
  • Introduced the UsedMemoryDecreasedComparator class to compare DatasetMemoryUsage objects based on the minimum difference in used memory. (UsedMemoryDecreasedComparator.java)
  • Updated the UsedMemoryIncreasedComparator class to compare DatasetMemoryUsage objects based on the maximum difference in used memory, reversing the comparison for sorting purposes. (UsedMemoryIncreasedComparator.java)

Validation Performance Collection Refactor:

  • Refactored the ValidationPerformanceCollector class to remove the use of BoundedPriorityQueue and instead use lists for memory usage tracking. (ValidationPerformanceCollector.java)
  • Consolidated the performance metrics calculation into a new PerformanceMetrics class and added methods to compute average, median, standard deviation, min, and max values. (ValidationPerformanceCollector.java) [1] [2]
  • Simplified the generation of performance metrics logs by introducing a helper method generatePerformanceMetricsLog to format and append metrics to the log string. (ValidationPerformanceCollector.java) [1] [2]

These changes aim to improve the maintainability and readability of the memory usage comparison logic and enhance the performance metrics reporting in the output-comparator module.

@davidgamez davidgamez marked this pull request as ready for review October 7, 2024 20:45
@davidgamez davidgamez added the do not merge This PR needs more work/discussion or is not meant to be merged label Oct 7, 2024
@davidgamez davidgamez changed the title Test/large feeds test: large feeds with ci pipeline Oct 7, 2024
Copy link
Contributor

github-actions bot commented Oct 9, 2024

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

@MobilityData MobilityData deleted a comment from github-actions bot Oct 10, 2024
@MobilityData MobilityData deleted a comment from github-actions bot Oct 10, 2024
@MobilityData MobilityData deleted a comment from github-actions bot Oct 10, 2024
@MobilityData MobilityData deleted a comment from github-actions bot Oct 10, 2024
@MobilityData MobilityData deleted a comment from github-actions bot Oct 10, 2024
@MobilityData MobilityData deleted a comment from github-actions bot Oct 10, 2024
@davidgamez davidgamez changed the title test: large feeds with ci pipeline feat: add memory metrics to acceptance tests Oct 11, 2024
@MobilityData MobilityData deleted a comment from github-actions bot Oct 11, 2024
@davidgamez davidgamez removed the do not merge This PR needs more work/discussion or is not meant to be merged label Oct 11, 2024
Copy link
Contributor

📝 Acceptance Test Report

📋 Summary

✅ The rule acceptance has passed for commit 265d9a2
Download the full acceptance test report here (report will disappear after 90 days).

📊 Notices Comparison

New Errors (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

Dropped Errors (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

New Warnings (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

Dropped Warnings (0 out of 1588 datasets, ~0%) ✅

No changes were detected due to the code change.

🛡️ Corruption Check

0 out of 1588 sources (~0 %) are corrupted.

⏱️ Performance Assessment

📈 Validation Time

Assess the performance in terms of seconds taken for the validation process.

Time Metric Dataset ID Reference (s) Latest (s) Difference (s)
Average -- 3.98 4.03 ⬆️+0.05
Median -- 1.39 1.41 ⬆️+0.03
Standard Deviation -- 11.34 11.50 ⬆️+0.16
Minimum in References Reports us-oregon-high-desert-point-gtfs-636 0.50 0.56 ⬆️+0.06
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 290.38 295.16 ⬆️+4.78
Minimum in Latest Reports us-california-catalina-express-gtfs-299 0.52 0.50 ⬇️-0.02
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 290.38 295.16 ⬆️+4.78
📜 Memory Consumption
Metric Dataset ID Reference (s) Latest (s) Difference (s)
Average -- 480.91 MiB 489.40 MiB ⬆️+8.49 MiB
Median -- 245.34 MiB 248.00 MiB ⬆️+2.67 MiB
Standard Deviation -- 854.66 MiB 880.42 MiB ⬆️+25.76 MiB
Minimum in References Reports tr-kocaeli-metro-izmir-gtfs-1824 34.05 MiB 34.10 MiB ⬆️+48.00 KiB
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 10.00 GiB 9.82 GiB ⬇️-185.38 MiB
Minimum in Latest Reports us-california-flex-v2-developer-test-feed-1-gtfs-1817 34.05 MiB 34.05 MiB ⬇️0 bytes
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 10.00 GiB 9.82 GiB ⬇️-185.38 MiB

@MobilityData MobilityData deleted a comment from github-actions bot Oct 15, 2024
@davidgamez davidgamez requested a review from cka-y October 15, 2024 17:29
Copy link
Contributor

📝 Acceptance Test Report

📋 Summary

✅ The rule acceptance has passed for commit 940ff27
Download the full acceptance test report here (report will disappear after 90 days).

📊 Notices Comparison

New Errors (0 out of 1601 datasets, ~0%) ✅

No changes were detected due to the code change.

Dropped Errors (0 out of 1601 datasets, ~0%) ✅

No changes were detected due to the code change.

New Warnings (0 out of 1601 datasets, ~0%) ✅

No changes were detected due to the code change.

Dropped Warnings (0 out of 1601 datasets, ~0%) ✅

No changes were detected due to the code change.

🛡️ Corruption Check

0 out of 1601 sources (~0 %) are corrupted.

⏱️ Performance Assessment

📈 Validation Time

Assess the performance in terms of seconds taken for the validation process.

Time Metric Dataset ID Reference (s) Latest (s) Difference (s)
Average -- 3.99 4.04 ⬆️+0.05
Median -- 1.39 1.41 ⬆️+0.03
Standard Deviation -- 11.44 11.44 ⬆️+0.00
Minimum in References Reports us-oregon-hut-airport-shuttle-gtfs-635 0.52 0.59 ⬆️+0.07
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 295.53 295.97 ⬆️+0.44
Minimum in Latest Reports us-massachusetts-massachusetts-area-express-max-gtfs-431 0.60 0.52 ⬇️-0.07
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 295.53 295.97 ⬆️+0.44
📜 Memory Consumption
Metric Dataset ID Reference (s) Latest (s) Difference (s)
Average -- 485.51 MiB 483.42 MiB ⬇️-2.10 MiB
Median -- 246.06 MiB 248.08 MiB ⬆️+2.02 MiB
Standard Deviation -- 879.81 MiB 874.43 MiB ⬇️-5.38 MiB
Minimum in References Reports tr-kocaeli-metro-izmir-gtfs-1824 34.05 MiB 34.05 MiB ⬆️+8.00 KiB
Maximum in Reference Reports gb-unknown-uk-aggregate-feed-gtfs-2014 10.00 GiB 10.21 GiB ⬆️+210.13 MiB
Minimum in Latest Reports us-oregon-hut-airport-shuttle-gtfs-635 34.06 MiB 34.05 MiB ⬇️-16.00 KiB
Maximum in Latest Reports gb-unknown-uk-aggregate-feed-gtfs-2014 10.00 GiB 10.21 GiB ⬆️+210.13 MiB

Copy link
Contributor

@cka-y cka-y left a comment

Choose a reason for hiding this comment

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

LGTM -- it made the memory consumption much easier to read

@davidgamez davidgamez merged commit 7426549 into master Oct 16, 2024
335 checks passed
@davidgamez davidgamez deleted the test/large-feeds branch October 16, 2024 14:50
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.

2 participants