Reduce impact of Reactive SQL client connection "stealing" on Hibernate Reactive performance

[yrodiere]

Description

@franz1981 and @Sanne have been investigating performance of Hibernate Reactive, and came to the conclusion that a specific problem has a significant impact on performance: using in one event loop a DB connection that was created by Vert.x for a different event loop.

This would be specifically this code: https://github.com/eclipse-vertx/vert.x/blob/c85bacac22166313d639411ad503eb38f5e2e3cc/vertx-core/src/main/java/io/vertx/core/internal/pool/SimpleConnectionPool.java#L602-L612

This issue tracks work on improving performance of this specific code.

Implementation ideas

One idea that floated around was to wait a bit for a connection to be available in the current context (event loop) before resorting to a connection from another context, thereby favoring use of connections from the same context.

Arguably, this would increase latency, but it would reduce context switching and thus resource utilization, so it could allow more parallel requests for the same resources.

[quarkus-bot]

/cc @DavideD (hibernate-reactive), @gavinking (hibernate-reactive), @tsegismont (reactive-sql-clients), @vietj (reactive-sql-clients)

[yrodiere]

@tsegismont Feel free to suggest another name than "stealing"; I know you think it's inappropriate but I don't have a better idea.

[franz1981]

Another implementation improvement idea is to be able to snoop in flight connection releases while performing the borrowing of connection; right now it checks connection presence without fully processing first all the releases, meaning that the decision could happen in an suboptimal way.

[yrodiere]

Note this issue is probably blocked at the moment; we need @franz1981 to confirm to us which benchmark to run to reproduce the problem exactly.

In the meantime, we can work on #46818, which will probably be necessary to evaluate success anyway.

[tsegismont]

Useful notes

1/ Policies available for connection creation

There are two policies:

• use the event loop of the caller at pool creation time,
• use as many event loops as configured in pool options.

This is where it happens: https://github.com/eclipse-vertx/vertx-sql-client/blob/abee64760761e491df5e06c77b34ba53f7461c4e/vertx-sql-client/src/main/java/io/vertx/sqlclient/impl/pool/SqlConnectionPool.java#L92-L112

In Quarkus, we use the second approach, with the number of event loops equal to the computed value of Quarkus:

quarkus/extensions/reactive-mysql-client/runtime/src/main/java/io/quarkus/reactive/mysql/client/runtime/MySQLPoolRecorder.java

Lines 177 to 181 in df55923

	if (dataSourceReactiveRuntimeConfig.eventLoopSize().isPresent()) {
	poolOptions.setEventLoopSize(Math.max(0, dataSourceReactiveRuntimeConfig.eventLoopSize().getAsInt()));
	} else if (eventLoopCount != null) {
	poolOptions.setEventLoopSize(Math.max(0, eventLoopCount));
	}

2/ Policy available for connection assignment

• Try reusing an existing connection with the same Vert.x context
• Try creating a connection
• Try reusing an existing connection from another Vert.x context
• Fall in waiters list

https://github.com/eclipse-vertx/vert.x/blob/79edb86aa10f64a0c0c970dc329a79e0f203b879/vertx-core/src/main/java/io/vertx/core/internal/pool/SimpleConnectionPool.java#L559

Not so much of a policy since it's hardcoded...

3/ Check if context assigned to connection can be retrieved

Yes, you can.

https://github.com/eclipse-vertx/vertx-sql-client/blob/167d208f7202e9e94ce94a7872bee459affbfad3/vertx-sql-client/src/main/java/io/vertx/sqlclient/impl/SocketConnectionBase.java#L128

[Sanne]

To add some more details of the current issue:

In the initial status just after bootstrap ("clean slate"), it's a corner case to have to shift the work to a different thread.

A corner case, yet very likely to happen in practice - so it will happen soon after the app starts processing things. As soon as it's happened a few times, having "stolen" connection availability from a different thread it makes it more likely for that thread to be starving for resources as well.

So each thread will start stealing from others with increasing likelihood, and there is no mechanism to reduce the likelihood or converge back to the good affinity .. so as processing progresses we keep getting worse and worse. That's the theory we have - lack of metrics makes it unconfirmed but looking at the code this seems quite plausible and we have some form of confirmation from the quite disappointing performance characteristics.

While I agree that there should be a strong(er) preference to use exclusively connections belonging to the current eventloop, we also need to allow (but not as preference) to use a connection from a different eventloop to ensure liveness; perhaps this shouldn't be allowed at all but as long as we can guarantee to not enter in a deadlock.

Proper balancing of the amount of available physical connections is also a concern: in a syntethic benchmark it's likely a reasonable choice to split the number of connections evenly across eventloops but in some more complex real-world applications this might be undesireable, so perhaps there's a need for rebalancing the strict number of physical connections we allow to a DB (which needs to be capped) between event loops; such re-balancing operations should have strong resistance though, we want to be able to adapt somewhat but don't want to be flip-flopping state too much.