On Mon, 15 Dec 2025 16:32:23 GMT, Jan Lahoda <[email protected]> wrote:
>> I think in the compiler you can't assume single threaded-ness, and unless >> this adds a deadlock risk (which I am pretty sure it doesn't) it's >> definitely my preference to have correctness and avoid weird bugs/failures. > > One instance of javac (i.e. one result of > `ToolProvider.getSystemJavaCompiler().getTask(...)` is single-threaded, and > cannot even be passed to a different thread without an external > synchronization. There's no need for synchronization inside single instance > of javac. > > The case of file managers is a bit different: some backend data structures > (like `JRTIndex` as such) are shared across various instances of file > managers, and those definitely need to be synchronized. To what degree > synchronization is needed here is unclear to me. The javadoc says that > multi-threaded access to file managers is not required, and I don't think > that concurrent use of the `JavacFileManager` from multiple instances of > javac is quite realistic (settings like multi-release or preview setting are > kept in fields in the file manager, and the different instances of javac > would talk past each other). > > I.e. I don't think the synchronization here is really necessary. OTOH, the > `JavacFileManager.getJRTIndex()` is currently synchronized, and it would seem > more consistent for the `getJRTIndex()` and `closeables` to share the same > synchronization approach. I would agree with Maurizio that I would mildly > prefer to drop the synchronization, but I can live with the synchronization. I prefer, for the sake of future maintainers, to make synchronization consistent. If one bit of code thinks locking is needed, then locking should be applied correctly everywhere to that instance. Not doing that means that, in a codebase which can be "light on comments", the confusion caused by seeing some bits of code caring about locking, and some not, could cost hours or even days of a maintainer's time as they try to untangle the inferred logic around synchronization. Worse it could lead to extremely rare and hard to debug problems. Remember that what's assumed to be single threaded today might not be so in future and future maintainers may be much less familiar with the code than you are. I the absence of a concrete clad argument that synchronisation is absolutely unnecessary (and in which case I want to remove it *everywhere* for this data structure and add comments explaining why that's safe) I strongly prefer to make the code more self-describing by making locking behaviour consistent. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/1761#discussion_r2622542560
