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

Bad performance locking #1688

Open
l3r8yJ opened this issue Aug 20, 2023 · 5 comments · May be fixed by #1689
Open

Bad performance locking #1688

l3r8yJ opened this issue Aug 20, 2023 · 5 comments · May be fixed by #1689

Comments

@l3r8yJ
Copy link

l3r8yJ commented Aug 20, 2023

actually this code is not parallel at all because of the synchronized keyword, I suggest using reentrant lock, wdyt?

synchronized (this.mutex) {

@fabriciofx
Copy link
Contributor

@l3r8yJ please, submit a PR!

@l3r8yJ
Copy link
Author

l3r8yJ commented Aug 20, 2023

@fabriciofx sure

@l3r8yJ l3r8yJ linked a pull request Aug 20, 2023 that will close this issue
@victornoel
Copy link
Collaborator

victornoel commented Aug 21, 2023

Hello @l3r8yJ, IMO this is actually incorrect, see my comment in the PR (#1689 (comment)).

That being said, the change is maybe anyway relevant (see https://stackoverflow.com/a/11821900), but let's be first clear about what it provides so that it can be documented and maybe applied more generally in the code base.

@l3r8yJ
Copy link
Author

l3r8yJ commented Aug 21, 2023

@victornoel Hi. I didn't fully understand what IMO means, but you can look up the benefits here

@victornoel
Copy link
Collaborator

victornoel commented Aug 21, 2023

@l3r8yJ IMO means in my opinion (as the ARC of this project ;).

The link you gave is the same I gave and it does not support at all the claims that "code is not parallel" (whatever that means...). Just that there is one advantage in our situation: support for Java 21 Virtual Threads. To be precise, the code change you propose:

  • is not unstructured
  • does not use polling nor fairness
  • it's not clear if it's more scalable

The only relevant advantages I see are:

  • Java 21 virtual threads support
  • interruptibility

Those are good reasons, but has nothing to do with the description of the opened ticket. If you agree with this conclusion, I will add a comment in the PR to take this discussion into account so that we can record those conclusions. If you have other advantages in mind please share them so that we can also record them.

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 a pull request may close this issue.

3 participants