Closed vitaly-ivanov closed 5 days ago
:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 91.95%. Comparing base (
b37c566
) to head (2af17cc
). Report is 203 commits behind head on main.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for this PR. Any chance you add a small unit / performance test to this to prevent regressions?
Thanks for this PR. Any chance you add a small unit / performance test to this to prevent regressions?
I think adding performance tests as unit tests may not be a good practice due to the execution time. I see that the benchmarks are in the separate repository https://github.com/datafaker-net/datafaker-benchmark. Should I add a new benchmark there?
yep, benchmark might be a good idea
Thanks for this PR. Any chance you add a small unit / performance test to this to prevent regressions?
I think adding performance tests as unit tests may not be a good practice due to the execution time. I see that the benchmarks are in the separate repository https://github.com/datafaker-net/datafaker-benchmark. Should I add a new benchmark there?
We have a few lightweight performance tests as part of the build, so it depends a bit on how long the test run is. If it's 10 seconds, it should be fine to add it to this build, if it's more then that, perhaps Datafaker-benchmark might be better.
I have added a new banchmark: https://github.com/datafaker-net/datafaker-benchmark/pull/25
Before changes:
Benchmark Mode Cnt Score Error Units
LocalePerformanceBenchmark.en_fullname thrpt 10 2700.254 ± 161.029 ops/ms
LocalePerformanceBenchmark.en_gb_fullname thrpt 10 12.079 ± 1.952 ops/ms
LocalePerformanceBenchmark.en_gb_streetAddress thrpt 10 11.230 ± 1.599 ops/ms
LocalePerformanceBenchmark.en_streetAddress thrpt 10 2045.465 ± 70.737 ops/ms
After changes:
Benchmark Mode Cnt Score Error Units
LocalePerformanceBenchmark.en_fullname thrpt 10 2645.880 ± 216.553 ops/ms
LocalePerformanceBenchmark.en_gb_fullname thrpt 10 38.643 ± 3.966 ops/ms
LocalePerformanceBenchmark.en_gb_streetAddress thrpt 10 51.417 ± 11.488 ops/ms
LocalePerformanceBenchmark.en_streetAddress thrpt 10 1909.792 ± 213.204 ops/ms
It gets a little better, but the difference is still significant for some reason.
@bodiam @snuyanzin Could you please release DF 2.3.1 with this fix? I heard complains about performance of DS 2.3.0 (that became obvious after we fixed the memory leaks).
Yes, can do, but I was hoping to release this one too: https://github.com/datafaker-net/datafaker/issues/1281
Overview
This is a partial fix for the issue https://github.com/datafaker-net/datafaker/issues/1285.
Result before changes:
Result after changes:
Result with no locale:
Results are based on the code here: https://github.com/vitaly-ivanov/datafaker-memory-leak/blob/main/app/src/main/kotlin/org/example/Performance.kt
Details
Nulls were interpreted as no attempt to load
If values were missing for the specified locale, the
loadValues
method returned null instead of an empty map, resulting in subsequent attempts to load values into the cache.