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

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

Closed
wants to merge 5 commits into from

Conversation

asolntsev
Copy link
Collaborator

@asolntsev asolntsev commented Jul 21, 2024

it's a simplified version of what happens with FakeValuesService.MAP_OF_METHOD_AND_COERCED_ARGS.

@snuyanzin Can you help with fixing CopyOnWriteMap?

@asolntsev asolntsev marked this pull request as draft July 21, 2024 11:14
@asolntsev asolntsev linked an issue Jul 21, 2024 that may be closed by this pull request
Copy link

what-the-diff bot commented Jul 21, 2024

PR Summary

  • New Test Class Added
    A new test class named CopyOnWriteMapTest.java has been added. The primary function of this class is to thoroughly check and determine the performance and reliability of multi-threaded, simultaneous data access in a specific object class called CopyOnWriteMap.

  • Introduction of concurrentPutAndGet() Method
    This method is a part of the new test class. It is specifically designed to carry out 'put' and 'get' actions on the CopyOnWriteMap object in a concurrent environment. Put simply, it tests if multiple threads can simultaneously and safely access and modify the data within this object type.

The overall goal of these changes is to ensure that our CopyOnWriteMap operates robustly and correctly under situations where multiple processes try to access and alter data at once.

@asolntsev asolntsev self-assigned this Jul 21, 2024
it's a simplified version of what happens with FakeValuesService.MAP_OF_METHOD_AND_COERCED_ARGS
Instead of two separate calls `putIfAbsent` and `get`, now we use `computeIfAbsent`.
Before this change, `get` sometimes returned null (apparently, because of GC or parallel threads or something similar).
…ervice

Instead of three separate calls `containsKey`, `get` and `put`, now we use a single `computeIfAbsent`.
this partially reverts commit 43e1fff
@asolntsev asolntsev marked this pull request as ready for review July 29, 2024 19:09
@bodiam
Copy link
Contributor

bodiam commented Oct 13, 2024

Hi @asolntsev , how about getting rid of the COWMap? I just did a performance test with Spring Boot native, and I did about 300.000 requests, and in my "production" code I'm getting nullpointers. For some reason, I got more NPEs in GraalVM than while using a JVM. If you want, you can easily reproduce it here by running the k6 performance test: https://github.com/datafaker-net/datafaker-native-demo

@kingthorin
Copy link
Collaborator

Wasn't the COWMap originally introduced to address performance issues?

@bodiam
Copy link
Contributor

bodiam commented Oct 13, 2024

@kingthorin I believe the idea is that it has some benefits regarding performance, but at the cost that the library no longer works reliably.

@asolntsev
Copy link
Collaborator Author

how about getting rid of the COWMap?

Yes, I like the idea.
I even tried to do it in #1203

The argument against it was that COWMap is faster. But I never checked this statement by myself. And I don't think this speed is really important.

@kingthorin
Copy link
Collaborator

Here is where it was introduced (I believe), it has further details.

#770

@kingthorin
Copy link
Collaborator

kingthorin commented Oct 13, 2024

Will using a synchronized map alleviate the issue? I know that might cause a slight performance hit but hopefully it still facilitates the original case.

Edit: or ConcurrentHashMap

Instead of two-three separate calls like `containsKey`, `get` and `put`, now we use a single `computeIfAbsent`.
@asolntsev
Copy link
Collaborator Author

@kingthorin @bodiam I investigated the problem a bit more.

Conclusion:
the problem is not in CopyOnWriteMap, but it's used.

When we have map of maps, then we often call get, then if != null and put. This sequence doesn't work well in concurrent execution by multiple threads. Instead, we should use computeIfAbsent.

I submitted PR #1381 that should fix our flaky problem.

<option name="Make" enabled="true" />
</method>
</configuration>
</component>
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this file?

@asolntsev
Copy link
Collaborator Author

The problem was fixed in PR #1381.
This PR is not needed anymore.

@asolntsev asolntsev closed this Oct 16, 2024
@asolntsev asolntsev deleted the reproduce/1306-flaky-bug branch October 16, 2024 20:58
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.

Unable to resolve expression during build
3 participants