Avoid ThreadLocals while using VirtualThreads #39696
Comments
|
This is the reduced list, ignoring things which belong to OpenJDK instead, and we have not much control of: |
geoand
added
area/virtual-threads
|
/cc @cescoffier (virtual-threads), @ozangunalp (virtual-threads) |
|
The vertx one seems to be triggered by |
|
@vietj I see that despite starting with your solution at
makes sense to me: the good thing is that on virtual threads (and the blocking thread pool as well) we can always await while asking this to be executed on the "right" event loop. I believe is better for both cases, but let's hear @cescoffier because maybe I'm missing something there |
|
Specifically for the ArC part, we don't do anything similar to Jackson. That is, we don't use thread locals as a terrible replacement for object pooling. @mkouba We could also put these objects into the |
|
I will try to think about the pool, but I think we should not use the vertx pool like this from a virtual thread too :-) |
|
@vietj yep I think the same: we could just enqueue a task to the right I/O thread (or - better - the one from which the request was coming). IIRC Virtual Threads on Quarkus already have a limited concurrency (using a |
|
@franz1981 in pool impl: public Future<SqlConnection> getConnection() {
ContextInternal current = vertx.getOrCreateContext();
if (pipelined) {
return current.failedFuture("Cannot acquire a connection on a pipelined pool");
}
Promise<SqlConnectionPool.PooledConnection> promise = current.promise();
acquire(current, connectionTimeout, promise);
return promise.future().map(conn -> {
SqlConnectionInternal wrapper = driver.wrapConnection(current, conn.factory(), conn);
conn.init(wrapper);
return wrapper;
});
}we could check here whether it's vertx thread or not |
|
but TBH we should discuss the use case here, because I am not sure to undrstand :-) |
|
@franz1981 we could perhaps also change |
|
IDK, https://github.com/eclipse-vertx/vert.x/blob/adbe97600cc6215f15ce2fac629c89f93618ca8f/src/main/java/io/vertx/core/net/impl/pool/CombinerExecutor.java#L85 is already pretty complex. |
|
the combiner is designed to be usable from any thread, it's not tied to netty, it just needs a thread to operate, removing the thread local here will not make the code more complex I think. so perhaps it should be done before entering the pool and that is the code to question I think. |
|
We cannot, because of reentrant actions: if we don't use a ConcurrentHashMap<Thread..., > we have no way to bring the context of where we are, around. And CHM is super slow for the other cases. |
@Ladicek Well, things related to |
we need to understand when this pool is not accessed from event loop and the use cases |
The stack trace is which is triggered by https://github.com/franz1981/fortune-benchmark/blob/fix_new_quarkus/fortune-virtual-thread-postgresql/src/main/java/me/escoffier/fortune/virtual/reactive/postgresql/FortuneRepository.java#L51-L62 public List<Fortune> listAll() {
List<Fortune> list = new ArrayList<>();
return datasource.getConnection().flatMap(connection ->
connection.query(Statements.SELECT_ALL_FORTUNES).execute()
.map(rs -> {
for (Row row : rs) {
list.add(new Fortune(row.getInteger(0), row.getString(1)));
}
return list;
})
.onTermination().call(connection::close)
).await().indefinitely();
}where |
|
@FroMage I've taken a quick look at the Virtual Thread stacks and the ThreadLocal usage reported at smallrye/smallrye-context-propagation#424 is particularly bad here as well (mostly because
Adding @mariofusco here as well, which was working on that issue IIRC In short, not just the remove (which is not nice), but the |
|
I'm all for removing |
|
That's not a problem, I can help with that one @FroMage - or we can have (given that virtual thread(s) are not the most common usage yet) the option of using an hybrid data-structure |
|
Perfect topic to discuss at the f2f, much easier in person, unless this is urgent? |
|
+100 for F2F discussion on this, great idea (almost forgot we have the F2F :D ) |
|
@franz1981 I submitted a PR for ArC (#39837), so feel free to do whatever you wish with this issue. |
|
Many thanks @Ladicek let me know if I need to take a look there for some reason, and I will try (likely in the next days) |
|
Actually I might have been a bit too fast, I'll let you know :-) From performance perspective, I'm adding a field (one at the moment, but may expand to two) into |
That field is not initialized in most cases, or? |
|
That is correct, but it makes all the |
Hm, "bigger" in the sense that you'll need roughly 4 more bytes for each instance? I don't think you'll be able to spot the difference (except for microbenchmarks where you create millions of beans ofc ;-). |
|
Yeah, it is 4 bytes for each CC. It isn't much, but indeed, it is 4 more bytes for each created bean. Probably not too big of a deal. |
|
I ended up making that 2 fields for the CC, so it's 8 bytes more for each created bean. Otherwise, #39837 seems OK, so @franz1981 if you think that PR needs some care, please take a look. Thanks! |
|
#39837 was merged, so there are no thread-locals used in ArC anymore (with the exception of thread-local contexts, but by default, those are only used on non-Vert.x threads). |
|
Ladislav's PR was merged so ArC should not use thread locals at all... |
Similarly to FasterXML/jackson-core#919
in quarkus we have few parts which are still using thread locals
eg running the test https://github.com/franz1981/fortune-benchmark/blob/main/fortune-virtual-thread-postgresql/src/test/java/me/escoffier/fortune/virtual/reactive/postgresql/test/FortuneApiTest.java
adding the parameter
-Djdk.traceVirtualThreadLocals=trueshould report this stack trace (which ca be used in IDEA stack trace analyzer to fix the issues, one by one):
I think JDK 21 have a BUG on the mentioned property, because the stacks which contains
are absolutely fine.
This is a sign that most users didn't used this feature to make sure are not using thread locals on virtual thread(s) OR that loom is not that used (yet, which makes sense) OR that thread local(s) is not widely used (which is unlikely, really).
Instead, https://github.com/openjdk/jdk/blob/b9c76dedf4aa2248a5e561a535c9e3e181f7836a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java#L2799 shows that OpenJDK still have to fix its own
ThreadLocalusages as well, although, is not a big deal per-se, given that the number of class-loading operations should not keep on increasing during a run, in theory.The text was updated successfully, but these errors were encountered: