Make FakeValuesService thread safe by snuyanzin · Pull Request #770 · datafaker-net/datafaker

snuyanzin · 2023-04-16T21:29:26Z

The PR assumes that most of the time there is no writes to the maps and only reads.
~~In this case there could be used optimistic locks most of the time and cheap StampedLock 's~~

~~based on measurements there is overhead only about 10% for some of methods, for others there is no noticeable difference~~
e.g.
Datafaker_KotlinFakerBenchmark.kotlinFakerTest
before PR

Benchmark                                       Mode  Cnt    Score   Error  Units
Datafaker_KotlinFakerBenchmark.kotlinFakerTest  avgt   10  390.813 ± 2.512  ms/op

after (10% slower)

Benchmark                                       Mode  Cnt    Score    Error  Units
Datafaker_KotlinFakerBenchmark.kotlinFakerTest  avgt   10  431.599 ± 13.887  ms/op

DatafakerSimpleMethods.firstname
before

Benchmark                              Mode  Cnt      Score     Error   Units
DatafakerSimpleMethods.firstname      thrpt   10   6016.154 ± 247.178  ops/ms

after (about 10% slower)

Benchmark                              Mode  Cnt      Score      Error   Units
DatafakerSimpleMethods.firstname      thrpt   10   5330.902 ±   67.988  ops/ms

DatafakerSimpleMethods.fullname
before

Benchmark                              Mode  Cnt      Score     Error   Units
DatafakerSimpleMethods.fullname       thrpt   10   2524.751 ±  26.856  ops/ms

after (about 10% slower)

Benchmark                              Mode  Cnt      Score      Error   Units
DatafakerSimpleMethods.fullname       thrpt   10   2235.404 ±   67.011  ops/ms

UPD: a new approach passing added test is with COWMap

codecov-commenter · 2023-04-16T21:31:57Z

Codecov Report

Merging #770 (06743ef) into main (d1880c0) will decrease coverage by 0.28%.
The diff coverage is 78.26%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main     #770      +/-   ##
============================================
- Coverage     92.54%   92.27%   -0.28%     
- Complexity     2654     2662       +8     
============================================
  Files           286      287       +1     
  Lines          5274     5309      +35     
  Branches        553      552       -1     
============================================
+ Hits           4881     4899      +18     
- Misses          252      269      +17     
  Partials        141      141

Impacted Files	Coverage Δ
...ain/java/net/datafaker/internal/helper/COWMap.java	`50.00% <50.00%> (ø)`
.../java/net/datafaker/service/FakeValuesService.java	`84.51% <96.42%> (+0.52%)`	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kingthorin

Is it possible/practical to add tests?

The changes seem sane to me. I haven’t pulled and used them at all. Or, looked for any places updates might have been missed (I feel like other probably know the code base better than I do).

snuyanzin · 2023-04-16T22:30:07Z

that's still a tricky part to create a 100% reproducible test for failing non thread safe FakeValuesService...
will see this week if i able to make it or not

bodiam · 2023-04-17T04:15:57Z

Would this be something @rwong-gw could maybe help validate?

rwong-gw · 2023-04-20T21:46:16Z

For the past couple days, I have been using CI to run my Gatling tests every half hour with this version, and everything looks good, with no hanging or deadlocks.

One suggestion: StampedLockMap does not implement several methods in Java's Map API, and just throws an UnsupportedOperationException for them. This causes it to interact poorly with debuggers (see attached image from IntelliJ). You should implement at least the size and entrySet methods to get it to work nicely with IntelliJ, and it probably wouldn't hurt to implement at least the remaining non-mutating operations.

snuyanzin · 2023-04-21T12:35:01Z

makes sense, added implementation

rwong-gw · 2023-04-21T18:45:34Z

Thanks, I can now drill down into StampedLockMaps in IntelliJ's debugger:

BTW, you might want to add @Override annotations to the methods putIfAbsent, computeIfAbsent, and merge.

NarekDW · 2023-04-21T20:31:25Z

    private static final Logger LOG = Logger.getLogger("faker");

-    private final Map<SingletonLocale, FakeValuesInterface> fakeValuesInterfaceMap = new IdentityHashMap<>();
+    private final StampedLockMap<SingletonLocale, FakeValuesInterface> fakeValuesInterfaceMap = new StampedLockMap<>(IdentityHashMap::new);


As we don't use any StampedLockMap specific functions, isn't it better to keep type unchanged (as Map) ?

yes, for some of them it makes sense, for some not since there is updateNestedValue method specific for StampedLockMap

NarekDW · 2023-04-21T20:34:00Z

+            } finally {
+                lock.unlockRead(stamp);
+            }
+        }


Why don't you reuse doROOperation function here? Looks like a duplicate of code...

in fact it was kind of test version...
As measurement shows here it's that rare case when code duplication wins. With doROOperation it becomes 10% slower.... The reason is that it requires Supplier object which is generated during runtime. Without this method and this object generation there is no need for that extra object.

snuyanzin · 2023-04-23T12:33:45Z

Funny thing is that I finally adapted the test from #759 attached by @rwong-gw
And once I decreased sleep time it starts failing with IllegalMonitorStateException since it can not unlock locks ...

At the same time another option I had in mind was CopyOnWriteMaps (COWMap) which for some reason is not provided by java core.

So I replaced it with COWMap and the problem has gone.
Another good thing that it is a bit faster than with stamplockmaps (almost same speed as with unsafe option).
Probably at the beginning it could consume a bit more memory.

@kingthorin so at least one test is added
@rwong-gw could you please once again double check your case if it is still ok? I'm asking since as mentioned above I replaced it with COWMap because of IllegalMonitorStateException

NarekDW · 2023-04-23T20:09:02Z

+    }
+
+    @Override
+    public V put(K key, V value) {


I think this function should be synchronized otherwise it might produce incorrect result, don't you think?
For example 2 threads will access this function at the same time, one of the pair of key-value will be lost...

No, that's the main idea of COW approach: it is lock free.
Ok let's consider the case when 2 threads accessing this map.
Yes, in worst case one of the value will be lost. However it is ok.
We use this as a cache. It means that the lost value will be recalculated and put into map next time.
So there is no issue.

NarekDW · 2023-04-23T20:11:06Z

+    }
+
+    @Override
+    public V remove(Object key) {


The same, don't you think that all the modification functions should be synchronized ?

see the answer above

NarekDW · 2023-04-23T20:11:18Z

+    }
+
+    @Override
+    public void putAll(Map<? extends K, ? extends V> m) {


synchronized ?

see the answer above

NarekDW · 2023-04-23T20:11:59Z

+    }
+
+    @Override
+    public void clear() {


synchronized

see the answer above

NarekDW · 2023-04-23T20:15:46Z

+        Map<K, V> newMap = mapSupplier.get();
+        newMap.putAll(map);
+        final V result = newMap.remove(key);
+        map = newMap;


Isn't it better to keep the internal map as unmodifiable? Just for safety.
Like:

map = Collections.unmodifiableMap(newMap);

rwong-gw · 2023-04-24T17:37:19Z

@rwong-gw could you please once again double check your case if it is still ok? I'm asking since as mentioned above I replaced it with COWMap because of IllegalMonitorStateException

@snuyanzin I'll give it a try.

snuyanzin · 2023-05-02T06:30:47Z

merging since it contains a test provided at #759
it was passed more than 30 times in a row.
There was only one failure not related to current issue (related to github actions)

snuyanzin mentioned this pull request Apr 16, 2023

FakeValuesService is not thread-safe #759

Closed

kingthorin reviewed Apr 16, 2023

View reviewed changes

NarekDW reviewed Apr 22, 2023

View reviewed changes

snuyanzin changed the title ~~Introduce StampedLock maps and make FakeValuesService thread safe~~ Make FakeValuesService thread safe Apr 23, 2023

snuyanzin requested review from NarekDW and kingthorin April 23, 2023 12:42

NarekDW reviewed Apr 23, 2023

View reviewed changes

NarekDW approved these changes Apr 23, 2023

View reviewed changes

rwong-gw reviewed Apr 27, 2023

View reviewed changes

Comment thread src/main/java/net/datafaker/internal/helper/COWMap.java Outdated

snuyanzin force-pushed the FakeValuesService branch 2 times, most recently from 2883fcd to 9616a24 Compare April 29, 2023 19:35

snuyanzin requested a review from rwong-gw April 30, 2023 10:31

snuyanzin added 10 commits May 1, 2023 08:14

Introduce StampLock maps and make FakeValuesService thread safe

459c547

Remove unused private method

4441871

Implement ro methods for StampledLockMap

2b50acc

Make some vars final

16ad661

Address feedback

3a141af

Add comment about duplication

bab4698

Use COWmap instead of StamplockMap

3679d56

Update comment content

8d3e991

Address feedback

94bcc73

Move REGEXP2SUPPLIER_MAP to COWMap

51edde5

Use COWMap for resolver

03675f4

snuyanzin force-pushed the FakeValuesService branch from 06743ef to 03675f4 Compare May 1, 2023 06:19

snuyanzin merged commit a24747e into datafaker-net:main May 2, 2023

snuyanzin added a commit to snuyanzin/datafaker that referenced this pull request May 2, 2023

Make FakeValuesService thread safe (datafaker-net#770)

e83c272

snuyanzin mentioned this pull request May 2, 2023

[Backport-1.x] Make FakeValuesService thread safe (#770) #819

Merged

snuyanzin added a commit that referenced this pull request May 2, 2023

Make FakeValuesService thread safe (#770) (#819)

d9c2cc2

snuyanzin mentioned this pull request May 15, 2023

Make FakerRepeatabilityIntegrationTest running in same thread #837

Merged

kingthorin mentioned this pull request Oct 13, 2024

#1306 add test that reproduces the flaky problem with CopyOnWriteMap #1310

Closed

Conversation

snuyanzin commented Apr 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kingthorin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snuyanzin commented Apr 16, 2023

Uh oh!

bodiam commented Apr 17, 2023

Uh oh!

rwong-gw commented Apr 20, 2023

Uh oh!

snuyanzin commented Apr 21, 2023

Uh oh!

rwong-gw commented Apr 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snuyanzin Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snuyanzin commented Apr 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwong-gw commented Apr 24, 2023

Uh oh!

Uh oh!

snuyanzin commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

snuyanzin commented Apr 16, 2023 •

edited

Loading

codecov-commenter commented Apr 16, 2023 •

edited

Loading

kingthorin left a comment •

edited

Loading

snuyanzin Apr 23, 2023 •

edited

Loading