search

yegor256 / cactoos

Object-Oriented Java primitives, as an alternative to Google Guava and Apache Commons
https://www.cactoos.org
MIT License
736 stars 163 forks source link

Bad performance locking #1688

Open l3r8yJ opened 1 year ago

l3r8yJ commented 1 year ago

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

https://github.com/yegor256/cactoos/blob/9ee0dd1fef855129104fdbbf6e66d6eafd4bfb05/src/main/java/org/cactoos/scalar/Synced.java#L82

fabriciofx commented 1 year ago

@l3r8yJ please, submit a PR!

l3r8yJ commented 1 year ago

@fabriciofx sure

victornoel commented 1 year ago

Hello @l3r8yJ, IMO this is actually incorrect, see my comment in the PR (https://github.com/yegor256/cactoos/pull/1689#issuecomment-1685916383).

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 commented 1 year ago

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

victornoel commented 1 year ago

@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:

The only relevant advantages I see are:

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.