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

Fiber refactorings #2

Open
wants to merge 75 commits into
base: java21
Choose a base branch
from
Open

Fiber refactorings #2

wants to merge 75 commits into from

Conversation

bgprudhomme
Copy link
Member

No description provided.

@@ -40,7 +40,7 @@ public class RemoteRepositoryTest {
*/
public static void main(String[] args) throws Exception {

ExecutorService executor = Executors.newFixedThreadPool(30);
ExecutorService executor = Executors.newVirtualThreadPerTaskExecutor();
Copy link
Member Author

Choose a reason for hiding this comment

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

Hit by own non-JMH performance test

Copy link
Member

Choose a reason for hiding this comment

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

Please clarify.

Copy link
Member Author

Choose a reason for hiding this comment

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

This test does not use JMH; the test happens in the main method.

@@ -55,7 +55,7 @@ private static abstract class PerformanceBase {

private static final int MAX_INSTANCES = Integer.MAX_VALUE;
private static final int N_QUERIES = 100;
private final ExecutorService executor = Executors.newFixedThreadPool(30);
private final ExecutorService executor = Executors.newVirtualThreadPerTaskExecutor();
Copy link
Member Author

Choose a reason for hiding this comment

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

Hit by own non-JMH performance test

Copy link
Member

Choose a reason for hiding this comment

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

Please clarify.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto the above

@@ -29,7 +29,7 @@ public class SparqlRepositoryTest {

public static void main(String[] args) throws Exception {

ExecutorService executor = Executors.newFixedThreadPool(20);
ExecutorService executor = Executors.newVirtualThreadPerTaskExecutor();
Copy link
Member Author

Choose a reason for hiding this comment

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

Hit by own non-JMH performance test

} finally {
semaphore.release();
}
} catch (InterruptedException e) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this exception necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I got an error before adding it, so I think yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

"InterruptedException must be caught or declared to be thrown"

return executorService.submit(runnable);
} catch (InterruptedException e) {
e.printStackTrace();
return CompletableFuture.completedFuture(null);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look correct and could use comments for clarification.

Copy link
Member Author

Choose a reason for hiding this comment

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

@khatchad This attempted change is using proposed approach 2 from this comment above, and in fact highlights a potential weakness of this approach. Here, because the submission is returned, we need to have a return statement in the catch block as well as in the try block.
Above, I chose to return the CompleteableFuture equivalent of null in the catch block since if the current thread gets interrupted before acquiring the semaphore and we allow the submission to happen anyway, the semaphore won't account for this submission and so more concurrent requests might be allowed than the intended maximum. However, I do acknowledge that this has the effect of preventing a planned submission to the executor from taking place, which would cause the actual number of submissions to be lower than expected in the event that an InterruptedException comes up in one or more threads.
If we want to always allow the submission to take place, two possible ways to use approach 2 that I see are:

(a): Return the submission at the end of the finally block

Future<?> submit(Runnable runnable) {
		try {
			semaphore.acquire();
		} catch (InterruptedException e) {
			e.printStackTrace();
		} finally {
			Future f = executorService.submit(runnable);
			semaphore.release();
			return f;
		}
	}

However, this requires me to create a variable in which to store the Future to which the submission evaluates so that we can wait until after submitting to release the semaphore, and because returning this future needs to happen at the end of the finally block.

(b): Submit at the end of both the try and catch blocks

Future<?> submit(Runnable runnable) {
		try {
			semaphore.acquire();
			return executorService.submit(runnable);
		} catch (InterruptedException e) {
			e.printStackTrace();
			return executorService.submit(runnable);
		} finally {
			semaphore.release();
		}
	}

This is redundant, and could potentially cause slowdown if too many concurrent requests are allowed due to the semaphore not accounting for submissions from the catch block. Also, a potentially bigger problem is that if a submission occurs from the catch block, this will be because the semaphore.acquire() call never acquired a permit, and thus the semaphore.release() call will increment semaphore's available permit count to one greater than it was before the overall submit() method call (instead of this count being the same before and after the call).

These concerns are tempting me towards choosing approach 1 from up here. What do you think?

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.

2 participants