Issue #61: Add Phaser in favor of a custom count and lock solution to Access and BufferLeaseImpl classes #70

eemhu · 2024-02-20T09:11:06Z

Note: This PR is a draft

…se> and Supplier<Lease>.

…re than 1 instead of zero as Phaser inits at 1, not 0.

kortemik · 2024-02-21T13:30:51Z

src/main/java/com/teragrep/rlp_03/context/frame/rental/Rental.java

+        phaser.arriveAndDeregister();
+    }
+
+    public boolean closed() {


is closed() extra?

kortemik · 2024-02-21T13:36:23Z

src/main/java/com/teragrep/rlp_03/context/buffer/BufferLeaseImpl.java

-        finally {
-            lock.unlock();
-        }
+        phaser.arriveAndDeregister();
    }

    @Override
    public boolean isRefCountZero() {


this must be changed so that it checks if the phaser isTerminated

fix 334d130

kortemik · 2024-02-21T13:37:27Z

src/main/java/com/teragrep/rlp_03/context/buffer/BufferLeaseImpl.java

@@ -28,13 +29,8 @@ public long id() {

    @Override
    public long refs() {


this should be removed or at least marked as unreliable because phaser is a lock free structure so ref count should be only used for debug logging

kortemik · 2024-02-21T13:38:28Z

src/main/java/com/teragrep/rlp_03/context/frame/rental/Rental.java

+public final class Rental implements AutoCloseable, Consumer<Lease>, Supplier<Lease> {
+
+    private final Phaser phaser;
+    private boolean closed;


this is extra, should return phaser.isTerminated() ?

return phaser.isTerminated: 334d130

kortemik · 2024-02-21T13:39:44Z

src/main/java/com/teragrep/rlp_03/context/frame/rental/Lease.java

@@ -1,12 +1,12 @@
-package com.teragrep.rlp_03.context.frame.access;
+package com.teragrep.rlp_03.context.frame.rental;

 public class Lease implements AutoCloseable {


could this be refactored so that it uses a sub-phaser that is registered to the parent?

kortemik · 2024-02-21T13:46:21Z

currently is broken, buffer is likely to be contaminated accross the threads:

[pool-9-thread-1] ERROR com.teragrep.rlp_03.context.RelpReadImpl - run() threw
java.lang.IllegalArgumentException: no match for EndOfTransfer character \n
        at com.teragrep.rlp_03.context.frame.function.EndOfTransferFunction.apply(EndOfTransferFunction.java:33)

i suspect the attemptRelease() is broken because clear() is not ensured by happens-before relationship of phaser.isTerminated()

…rk BufferLeaseImpl.refs as unreliable, change Rental.closed() to return phaser.isTerminated

kortemik · 2024-02-22T12:41:42Z

there are test failures now

Note: RentalTest contains IllegalStateException assert that does not seem to happen, commented out for now.

eemhu · 2024-02-23T08:52:29Z

seem to pass tests locally, however "RentalTest contains IllegalStateException assert that does not seem to happen, commented out for now." in 38ec3d0

add sub phaser to Lease

kortemik · 2024-02-23T17:51:01Z

Regarding BufferLeaseImpl, it might be appropriate to store cleared buffers in the pool and take new Leases (with new phasers) for them when taking out from the pool because phasers are terminated when returning. pe 23. helmik. 2024 klo 10.52 eemhu ***@***.***> kirjoitti:

…

seem to pass tests locally, however "RentalTest contains IllegalStateException assert that does not seem to happen, commented out for now." in 38ec3d0 <38ec3d0> add sub phaser to Lease — Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGX2OTGWTTALPQULM5SL4LYVBKFRAVCNFSM6AAAAABDQYGTNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQHE2DEMZSHE> . You are receiving this because you commented.Message ID: ***@***.***>

eemhu · 2024-02-26T10:43:55Z

Regarding BufferLeaseImpl, it might be appropriate to store cleared buffers in the pool and take new Leases (with new phasers) for them when taking out from the pool because phasers are terminated when returning. pe 23. helmik. 2024 klo 10.52 eemhu @.> kirjoitti:
…
seem to pass tests locally, however "RentalTest contains IllegalStateException assert that does not seem to happen, commented out for now." in 38ec3d0 <38ec3d0> add sub phaser to Lease — Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGX2OTGWTTALPQULM5SL4LYVBKFRAVCNFSM6AAAAABDQYGTNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQHE2DEMZSHE . You are receiving this because you commented.Message ID: @.>

@kortemik
Should BufferLeasePool.take() check the polled BufferLease for terminated Phaser and create a new BufferLeaseImpl in that case as well? Currently there is a null-check only.

… with the pre-existing buffer and id.

kortemik · 2024-02-27T09:46:53Z

I think it might be appropriate to use the decorator pattern for solving this. One would have a decorator class with the phaser and do new PhaserDecoratedBufferLeaseImpl(BufferLease bl) implements BufferLease. Please create it so that only the terminated ones return the encapsulated BufferLeaseImpl, otherwise a static stub is returned. This way I think the code retains the responsibility assignment principle. I wonder if you think this way too? ma 26. helmik. 2024 klo 12.44 eemhu ***@***.***> kirjoitti:

…

Regarding BufferLeaseImpl, it might be appropriate to store cleared buffers in the pool and take new Leases (with new phasers) for them when taking out from the pool because phasers are terminated when returning. pe 23. helmik. 2024 klo 10.52 eemhu *@*. *> kirjoitti: … <#m_-7089496626781693482_> seem to pass tests locally, however "RentalTest contains IllegalStateException assert that does not seem to happen, commented out for now." in 38ec3d0 <38ec3d0> <38ec3d0 <38ec3d0>> add sub phaser to Lease — Reply to this email directly, view it on GitHub <#70 (comment) <#70 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGX2OTGWTTALPQULM5SL4LYVBKFRAVCNFSM6AAAAABDQYGTNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQHE2DEMZSHE <https://github.com/notifications/unsubscribe-auth/AAGX2OTGWTTALPQULM5SL4LYVBKFRAVCNFSM6AAAAABDQYGTNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQHE2DEMZSHE> . You are receiving this because you commented.Message ID: @.*> Should BufferLeasePool.take() check the polled BufferLease for terminated Phaser and create a new BufferLeaseImpl in that case as well? Currently there is a null-check only. — Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGX2OQGEGMVRE4QFY63BODYVRRPPAVCNFSM6AAAAABDQYGTNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRTHAZDCNJYGM> . You are receiving this because you commented.Message ID: ***@***.***>

…rnalOffer, add stub unless phaser was terminated explicitly

eemhu · 2024-02-27T13:47:10Z

moved phaser to use decorator pattern PhaserDecoratedBufferLease which contains the phaser-related ops and BufferLeaseImplcontains the buffer itself.

kortemik · 2024-02-27T15:01:50Z

Does it differ much from the Rental+Lease? If we'd generify Rental would it fit? ti 27. helmik. 2024 klo 15.47 eemhu ***@***.***> kirjoitti:

…

moved phaser to use decorator pattern PhaserDecoratedBufferLease which contains the phaser-related ops and BufferLeaseImplcontains the buffer itself. — Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGX2OWL3FXV75F327DJ3ILYVXPWTAVCNFSM6AAAAABDQYGTNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRWGU4DQMJYHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

eemhu · 2024-02-28T12:54:51Z

TODO:

remove non-atomic operations from Phaser (registeredParties <= 1) etc.
remove addRef from bufferleasepool take() so the initial phaser registration counts as it
use PhaserDecoration "between" take() and offer(). Phaser is only included when bufferLease is in use. Terminate phaser.

Possibly:

use synchronized block instead of lock in buffer()
Rental as generic?

… to queue.

eemhu · 2024-02-29T08:25:16Z

removed comparisons from phaser related classes, added synchronized keyword to bufferLeaseImpl.buffer() and removed lock. Add phaser decoration when polling from queue and remove when adding back to queue.

eemhu · 2024-02-29T08:27:58Z

Perhaps the offer() method should take in type of PhaserDecoratedBufferLease so we can get rid of instanceof check?

kortemik · 2024-02-29T09:40:26Z

Yes, this could be changed so that it accepts only decorated ones, as they are the only type out there: public void offer(BufferLease bufferLease) { to 29. helmik. 2024 klo 10.28 eemhu ***@***.***> kirjoitti:

…

Perhaps the queue should take in type of PhaserDecoratedBufferLease so we can get rid of instanceof check? — Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGX2OU4FI3IY3T76IRQKWLYV3TBXAVCNFSM6AAAAABDQYGTNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZQGY2DGNRRG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…Lease to BufferContainer. Pool holds BufferContainers and wraps them in BufferLeases while out of the queue.

eemhu · 2024-02-29T10:16:17Z

Changed PhaserDecoratedBufferLease to be simply called BufferLease, and changed previous BufferLease to be BufferContainer. Now the pool will only contain BufferContainers, and they are wrapped in BufferLeases when taking them out of the pool.

kortemik · 2024-03-01T10:27:08Z

please separate the Access -> Rental to different pr, close this one and open new ones for these both.

kortemik · 2024-03-01T11:20:05Z

running with the a1534ed following agains the ManualPerformanceTest:

/opt/Fail-Safe/rsyslog/rsyslog/bin/tcpflood -p1601 -y -Trelp-plain -m100000000 -c 8 -Y -M "<167>1 2003-03-01T01:00:00.000Z mymachine.example.com tcpflood - tag [tcpflood@32473 MSGNUM="12857358"] msgnum:"

gives

[pool-9-thread-4] ERROR com.teragrep.rlp_03.context.buffer.BufferLeasePool - dirty buffer in pool, terminating

while main branch does not.

@StrongestNumber9 please check the rlp_09 pr tests so that they will reproduce this.

@eemhu please check why this differs from main branch.

this utility is provided from rsyslog project via fail-safe-rsyslog-8.2102.2-1558.fc34.x86_64.rpm, one can obtain it from nexus.

eemhu · 2024-03-01T13:19:00Z

adding synchronized to BufferLeaseImpl.attemptRelease() method seems to fix it. Perhaps multiple threads try to access attemptRelease() at the same time and thus accessing the buffer as well.

eemhu · 2024-03-01T13:23:46Z

would make sense considering the main branch version uses a lock there.

… to fix dirty buffer in pool.

StrongestNumber9 · 2024-03-05T10:57:18Z

running with the a1534ed following agains the ManualPerformanceTest:
/opt/Fail-Safe/rsyslog/rsyslog/bin/tcpflood -p1601 -y -Trelp-plain -m100000000 -c 8 -Y -M "<167>1 2003-03-01T01:00:00.000Z mymachine.example.com tcpflood - tag [tcpflood@32473 MSGNUM="12857358"] msgnum:"
gives
[pool-9-thread-4] ERROR com.teragrep.rlp_03.context.buffer.BufferLeasePool - dirty buffer in pool, terminating
while main branch does not.

@StrongestNumber9 please check the rlp_09 pr tests so that they will reproduce this.

@eemhu please check why this differs from main branch.

this utility is provided from rsyslog project via fail-safe-rsyslog-8.2102.2-1558.fc34.x86_64.rpm, one can obtain it from nexus.

Is this still relevant as

adding synchronized to BufferLeaseImpl.attemptRelease() method seems to fix it. Perhaps multiple threads try to access attemptRelease() at the same time and thus accessing the buffer as well.

eemhu added 4 commits February 19, 2024 15:54

add phaser to Access.java

ef7cb18

remove arriveAndAwaitAdvance calls, add phaser to BufferLeaseImpl.

2882b0e

rename Access class to Rental. Implements AutoCloseable, Consumer<Lea…

f8267cf

…se> and Supplier<Lease>.

more renames from 'access' to 'rental'. check if registrations are mo…

08e15e8

…re than 1 instead of zero as Phaser inits at 1, not 0.

eemhu marked this pull request as ready for review February 21, 2024 08:19

kortemik reviewed Feb 21, 2024

View reviewed changes

change BufferLeaseImpl.isRefCountZero to check phaser termination, ma…

334d130

…rk BufferLeaseImpl.refs as unreliable, change Rental.closed() to return phaser.isTerminated

eemhu added 2 commits February 23, 2024 08:46

fix attemptRelease condition.

38ec3d0

Note: RentalTest contains IllegalStateException assert that does not seem to happen, commented out for now.

add subPhaser to Lease.java

bba2af2

re-create BufferLeaseImpl in BufferLeasePool if phaser was terminated…

61b152a

… with the pre-existing buffer and id.

add PhaserDecoratedBufferLease and in BufferLeasePool use it. In Inte…

8169cb3

…rnalOffer, add stub unless phaser was terminated explicitly

eemhu added 2 commits February 29, 2024 09:52

remove comparisons from phaser. unwrap phaser decorator before adding…

9100506

… to queue.

add synchronized keyword to BufferLeaseImpl.buffer() to get rid of lock

1f422d9

eemhu added 2 commits February 29, 2024 11:50

replace instanceof check with isPhaserDecorated method

0cceb85

Change PhaserDecoratedBufferLease to be BufferLease and change Buffer…

9a6e836

…Lease to BufferContainer. Pool holds BufferContainers and wraps them in BufferLeases while out of the queue.

fix wrong logging in BufferLeasePool.take()

68097c6

kortemik added 2 commits March 1, 2024 13:09

do not add empty stub buffers to pool

0047aa7

FIXME termination of dirty state

a1534ed

kortemik marked this pull request as draft March 1, 2024 11:29

add 'synchronized' keyword to BufferLeaseImpl.attemptRelease() method…

e7c40c7

… to fix dirty buffer in pool.

eemhu closed this Mar 6, 2024

eemhu mentioned this pull request Mar 6, 2024

Issue #86: Add Phaser to Access #87

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #61: Add Phaser in favor of a custom count and lock solution to Access and BufferLeaseImpl classes #70

Issue #61: Add Phaser in favor of a custom count and lock solution to Access and BufferLeaseImpl classes #70

eemhu commented Feb 20, 2024 •

edited

Loading

kortemik Feb 21, 2024

kortemik Feb 21, 2024

eemhu Feb 22, 2024

kortemik Feb 21, 2024

kortemik Feb 21, 2024

eemhu Feb 22, 2024

kortemik Feb 21, 2024

kortemik commented Feb 21, 2024

kortemik commented Feb 22, 2024

eemhu commented Feb 23, 2024

kortemik commented Feb 23, 2024 via email

eemhu commented Feb 26, 2024 •

edited

Loading

kortemik commented Feb 27, 2024 via email

eemhu commented Feb 27, 2024

kortemik commented Feb 27, 2024 via email

eemhu commented Feb 28, 2024 •

edited

Loading

eemhu commented Feb 29, 2024

eemhu commented Feb 29, 2024 •

edited

Loading

kortemik commented Feb 29, 2024 via email

eemhu commented Feb 29, 2024

kortemik commented Mar 1, 2024

kortemik commented Mar 1, 2024 •

edited

Loading

eemhu commented Mar 1, 2024 •

edited

Loading

eemhu commented Mar 1, 2024

StrongestNumber9 commented Mar 5, 2024

		@@ -28,13 +29,8 @@ public long id() {

		@Override
		public long refs() {

Issue #61: Add Phaser in favor of a custom count and lock solution to Access and BufferLeaseImpl classes #70

Issue #61: Add Phaser in favor of a custom count and lock solution to Access and BufferLeaseImpl classes #70

Conversation

eemhu commented Feb 20, 2024 • edited Loading

kortemik Feb 21, 2024

Choose a reason for hiding this comment

kortemik Feb 21, 2024

Choose a reason for hiding this comment

eemhu Feb 22, 2024

Choose a reason for hiding this comment

kortemik Feb 21, 2024

Choose a reason for hiding this comment

kortemik Feb 21, 2024

Choose a reason for hiding this comment

eemhu Feb 22, 2024

Choose a reason for hiding this comment

kortemik Feb 21, 2024

Choose a reason for hiding this comment

kortemik commented Feb 21, 2024

kortemik commented Feb 22, 2024

eemhu commented Feb 23, 2024

kortemik commented Feb 23, 2024 via email

eemhu commented Feb 26, 2024 • edited Loading

kortemik commented Feb 27, 2024 via email

eemhu commented Feb 27, 2024

kortemik commented Feb 27, 2024 via email

eemhu commented Feb 28, 2024 • edited Loading

eemhu commented Feb 29, 2024

eemhu commented Feb 29, 2024 • edited Loading

kortemik commented Feb 29, 2024 via email

eemhu commented Feb 29, 2024

kortemik commented Mar 1, 2024

kortemik commented Mar 1, 2024 • edited Loading

eemhu commented Mar 1, 2024 • edited Loading

eemhu commented Mar 1, 2024

StrongestNumber9 commented Mar 5, 2024

eemhu commented Feb 20, 2024 •

edited

Loading

eemhu commented Feb 26, 2024 •

edited

Loading

eemhu commented Feb 28, 2024 •

edited

Loading

eemhu commented Feb 29, 2024 •

edited

Loading

kortemik commented Mar 1, 2024 •

edited

Loading

eemhu commented Mar 1, 2024 •

edited

Loading