Improve performance when generating with a specific locale

[vitaly-ivanov]

Overview

This is a partial fix for the issue #1285.

Result before changes:

Time to process 1000000 values: 40.341209708s

Result after changes:

Time to process 1000000 values: 19.826039958s

Result with no locale:

Time to process 1000000 values: 2.310795584s

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.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.95%. Comparing base (b37c566) to head (2af17cc).
Report is 203 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1286      +/-   ##
============================================
- Coverage     92.35%   91.95%   -0.41%     
- Complexity     2821     3086     +265     
============================================
  Files           292      310      +18     
  Lines          5609     6025     +416     
  Branches        599      627      +28     
============================================
+ Hits           5180     5540     +360     
- Misses          275      333      +58     
+ Partials        154      152       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[asolntsev]

LGTM

[bodiam]

Thanks for this PR. Any chance you add a small unit / performance test to this to prevent regressions?

[vitaly-ivanov]

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?

[snuyanzin]

yep, benchmark might be a good idea

[bodiam]

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.

[vitaly-ivanov]

I have added a new banchmark: datafaker-net/datafaker-benchmark#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.

[asolntsev]

@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).

[bodiam]

Yes, can do, but I was hoping to release this one too: #1281