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

Threading issues in PVAccess #51

Closed
shroffk opened this issue Apr 2, 2019 · 4 comments
Closed

Threading issues in PVAccess #51

shroffk opened this issue Apr 2, 2019 · 4 comments

Comments

@shroffk
Copy link
Contributor

shroffk commented Apr 2, 2019

1d300d3#commitcomment-32935258

Matej pointed out, that in PR #9 there are a few instances where the synchronization was used not just on the collection operations but on blocks of code around it.

So...
I can add synchronized blocks back where they are needed.

However, It might still be good to consider the use of java synchronized collections as a way to simplify the code.

@msekoranja
Copy link
Contributor

Simplified java synchronized collections are only good if you do simple operations on collections. Once you need to do a more-then-one operation as atomic, they do not work anymore (where not intended for that case).

The only thing that might looks non-standard are IntHashMap/ShortHashMap. They are there for performance reasons to avoid auto-boxing of type. I guess this is not a big deal and they can be replaces with standard collections - however syncrhonization must stay as it was.

@shroffk
Copy link
Contributor Author

shroffk commented Apr 2, 2019

I put together a small performance test to compare the performance of the

Attemp 0 Added a 100000 entries to IntHashMap in :21 ms
Attemp 0 Added a 100000 entries to synchronized hash map in :23 ms

Attemp 0 Randomly read 100000 entries from IntHashMap in :56 ms
Attemp 0 Randomly read 100000 entries from synchronized map in :19 ms

@shroffk
Copy link
Contributor Author

shroffk commented Apr 2, 2019

Simplified java synchronized collections are only good if you do simple operations on collections. Once you need to do a more-then-one operation as atomic, they do not work anymore (where not intended for that case).

Agreed, In the PR I see a few blocks where the synchronization was replaced. I will add the synchronization blocks back.

shroffk added a commit that referenced this issue Apr 2, 2019
if the entire method body is in a synchronized block and all methods use
the same lock (transports). I feel we could make the method itself
synchronized.
shroffk added a commit that referenced this issue Apr 2, 2019
@shroffk shroffk mentioned this issue Apr 2, 2019
@shroffk shroffk closed this as completed Apr 26, 2019
@shroffk shroffk reopened this Apr 26, 2019
@shroffk shroffk closed this as completed Apr 30, 2019
@shroffk
Copy link
Contributor Author

shroffk commented Apr 30, 2019

closing it for the time being, the broader issue about synchronization will be added as individual tickets

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

No branches or pull requests

2 participants