Re: RFR: 8297444: Refactor the javacserver build tool [v4]
> Now that the javacserver no longer has any ambitions outside being a > buildtool customized for the JDK build process, a lot of abstractions and > generalizations can be removed. > > This will allow the actual behavior to be more clearly visible, and will help > debugging the issues we are still seeing (most likely race conditions), and > to convert the tool to use the ToolProvider API in the future. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Use Server.class instead of hard-coding a string - Changes: - all: https://git.openjdk.org/jdk/pull/11298/files - new: https://git.openjdk.org/jdk/pull/11298/files/82998663..414c8ac1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11298&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11298&range=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11298.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11298/head:pull/11298 PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v3]
On Thu, 24 Nov 2022 20:36:06 GMT, Magnus Ihse Bursie wrote: >> Now that the javacserver no longer has any ambitions outside being a >> buildtool customized for the JDK build process, a lot of abstractions and >> generalizations can be removed. >> >> This will allow the actual behavior to be more clearly visible, and will >> help debugging the issues we are still seeing (most likely race conditions), >> and to convert the tool to use the ToolProvider API in the future. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typo > > Co-authored-by: Andrey Turbanov I have now tested a variety of different scenarios, with incremental rebuilds with and without modified files, with and without syntax errors, and also with JDK_FILTER. I have also inadvertently found a way to crash javac (!) and verified that the stack trace too propagated just as it should. :-) (I will file a bug for this on javac) - PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v3]
> Now that the javacserver no longer has any ambitions outside being a > buildtool customized for the JDK build process, a lot of abstractions and > generalizations can be removed. > > This will allow the actual behavior to be more clearly visible, and will help > debugging the issues we are still seeing (most likely race conditions), and > to convert the tool to use the ToolProvider API in the future. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Fix typo Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/11298/files - new: https://git.openjdk.org/jdk/pull/11298/files/2a0e0975..82998663 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11298&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11298&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/11298.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11298/head:pull/11298 PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
On Tue, 22 Nov 2022 23:40:30 GMT, Magnus Ihse Bursie wrote:
>> Now that the javacserver no longer has any ambitions outside being a
>> buildtool customized for the JDK build process, a lot of abstractions and
>> generalizations can be removed.
>>
>> This will allow the actual behavior to be more clearly visible, and will
>> help debugging the issues we are still seeing (most likely race conditions),
>> and to convert the tool to use the ToolProvider API in the future.
>
> Magnus Ihse Bursie has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Fix typo #2
>
>Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com>
> - Fix typo
>
>Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com>
make/langtools/tools/javacserver/server/Server.java line 69:
> 67: private static LazyInitFileLog errorLog;
> 68:
> 69: public static void main(String... args) {
Nit:
Suggestion:
public static void main(String... args) {
-
PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
On Tue, 22 Nov 2022 23:40:30 GMT, Magnus Ihse Bursie wrote: >> Now that the javacserver no longer has any ambitions outside being a >> buildtool customized for the JDK build process, a lot of abstractions and >> generalizations can be removed. >> >> This will allow the actual behavior to be more clearly visible, and will >> help debugging the issues we are still seeing (most likely race conditions), >> and to convert the tool to use the ToolProvider API in the future. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Fix typo #2 > >Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> > - Fix typo > >Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> Marked as reviewed by cstein (Committer). - PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
On Tue, 22 Nov 2022 23:32:51 GMT, Magnus Ihse Bursie wrote:
>> make/langtools/tools/javacserver/client/Client.java line 148:
>>
>>> 146: cmd.addAll(Arrays.asList(conf.javaCommand().split(" ")));
>>> 147: // javacserver.server.Server is the server main class
>>> 148: cmd.add("javacserver.server.Server");
>>
>> Did you consider referencing the class name using `Server.class.getName()`?
>
> No, can't say I have. Do you think there is anything to gain by doing that?
> (Apart from possibly getting this right automatically if the class is renamed
> by IDE tools) (I'm not sure it would be a win in readability)
I like to reference class names that way so that it's typed instead of just a
string. If the class changes name, you get a compilation error, and the IDE
would handle refactoring automatically. Not a strong need in this particular
instance, just something I generally tend to do.
-
PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
On Tue, 22 Nov 2022 23:15:20 GMT, Erik Joelsson wrote:
> Things like incremental builds, some forced failure conditions etc?
I tested simple incremental builds but I should probably make a bit more
advanced versions where I modify different files to trigger more real-life like
scenarios, including compilation errors. Good point.
> make/langtools/tools/javacserver/client/Client.java line 148:
>
>> 146: cmd.addAll(Arrays.asList(conf.javaCommand().split(" ")));
>> 147: // javacserver.server.Server is the server main class
>> 148: cmd.add("javacserver.server.Server");
>
> Did you consider referencing the class name using `Server.class.getName()`?
No, can't say I have. Do you think there is anything to gain by doing that?
(Apart from possibly getting this right automatically if the class is renamed
by IDE tools) (I'm not sure it would be a win in readability)
-
PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
> Now that the javacserver no longer has any ambitions outside being a > buildtool customized for the JDK build process, a lot of abstractions and > generalizations can be removed. > > This will allow the actual behavior to be more clearly visible, and will help > debugging the issues we are still seeing (most likely race conditions), and > to convert the tool to use the ToolProvider API in the future. Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision: - Fix typo #2 Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> - Fix typo Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/11298/files - new: https://git.openjdk.org/jdk/pull/11298/files/3dd8af2a..2a0e0975 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11298&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11298&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11298.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11298/head:pull/11298 PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool
On Tue, 22 Nov 2022 19:54:51 GMT, Magnus Ihse Bursie wrote:
> Now that the javacserver no longer has any ambitions outside being a
> buildtool customized for the JDK build process, a lot of abstractions and
> generalizations can be removed.
>
> This will allow the actual behavior to be more clearly visible, and will help
> debugging the issues we are still seeing (most likely race conditions), and
> to convert the tool to use the ToolProvider API in the future.
Only some minor comments. Looks good to me overall. The proof is in the
building.
Have you tried more than just full builds on all platforms? Things like
incremental builds, some forced failure conditions etc?
make/langtools/tools/javacserver/client/Client.java line 140:
> 138:
> 139: /*
> 140: * Fork a server process process and wait for server to come around
Suggestion:
* Fork a server process and wait for server to come around
make/langtools/tools/javacserver/client/Client.java line 148:
> 146: cmd.addAll(Arrays.asList(conf.javaCommand().split(" ")));
> 147: // javacserver.server.Server is the server main class
> 148: cmd.add("javacserver.server.Server");
Did you consider referencing the class name using `Server.class.getName()`?
make/langtools/tools/javacserver/server/Server.java line 200:
> 198:
> 199: // Set up logging for this thread. Stream back logging
> messages to
> 200: // client on the format format "level:msg".
Suggestion:
// client on the format "level:msg".
-
Marked as reviewed by erikj (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool
On Tue, 22 Nov 2022 19:54:51 GMT, Magnus Ihse Bursie wrote: > Now that the javacserver no longer has any ambitions outside being a > buildtool customized for the JDK build process, a lot of abstractions and > generalizations can be removed. > > This will allow the actual behavior to be more clearly visible, and will help > debugging the issues we are still seeing (most likely race conditions), and > to convert the tool to use the ToolProvider API in the future. The remaining changes are "pure" refactorings. I have moved code around, and reworked the abstraction layers. Basically, I put everything pertaining to the client in a long sequence (calling the client is basically a trivial sequence of events), and similarly for the server. Code that were shared between both were put in the `shared` or `util` packages (the latter for more auxiliary classes, the former for more central). Everything else were mercilessly removed. I then abstracted out what seemed like reasonable parts from the long sequences in Client and Server, in the process mostly recreating the thread pool and idle detection classes of the former structure (so git will present them as renames). I've been extra careful not to change any behavior regarding the portfile locking and synchronization, or the actual protocol exchanged by the server and client (with one exception: the exit code from javac is now returned as an integer, and not as a string representing the `Result` enum.) I believe there are a number of places where races can be introduced, and I intend to come back later and try to patch this. But I deemed it essential that I do not change any behavior in that regard in this patch, otherwise it would be hopeless to know if a potential future regression was caused by a mistake in my refactoring, or that any changed behavior was faulty. - PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool
On Tue, 22 Nov 2022 19:54:51 GMT, Magnus Ihse Bursie wrote: > Now that the javacserver no longer has any ambitions outside being a > buildtool customized for the JDK build process, a lot of abstractions and > generalizations can be removed. > > This will allow the actual behavior to be more clearly visible, and will help > debugging the issues we are still seeing (most likely race conditions), and > to convert the tool to use the ToolProvider API in the future. I realize this PR can be a bit of a challenge to review. I'll explain the changes I have done. The "actual" changes I have made is to how the javacserver tool is called from the makefiles, and how the client starts the server. The client now takes a `--conf=` argument, instead of `--server:conf=`. This argument has to be the first argument; the rest of the argument line is passed on to javac in the server. This allowed me to remove all the complex option processing stuff. Also, the client cannot any longer be called in "stand-alone" mode where no server is used (basically a complex version of plain `javac`). Secondly, the server is now started with a separate main method in the Server class. This means the entry point need not determine if it is being called as a client or a server, simplifying further. The server takes exactly one argument, the path of the port file. That means the last of the option processing could go. Third, the configuration file has changed slightly. Instead of a `servercmd` there is now a `javacmd`, which tells the client how to start java (suitable path to the java executable, and proper flags) when launching the server. The actual class used to launch the server is known by the client and does not have to be provided by the makefile. This change made new configuration files incompatible with old files; it turned out make had a bit of a problem of recognizing the file needed to be updated, so I chose a slightly different name for the configuration file, to avoid strange recompilation errors when this patch gets integrated. - PR: https://git.openjdk.org/jdk/pull/11298
RFR: 8297444: Refactor the javacserver build tool
Now that the javacserver no longer has any ambitions outside being a buildtool customized for the JDK build process, a lot of abstractions and generalizations can be removed. This will allow the actual behavior to be more clearly visible, and will help debugging the issues we are still seeing (most likely race conditions), and to convert the tool to use the ToolProvider API in the future. - Commit messages: - 8297444: Refactor the javacserver build tool Changes: https://git.openjdk.org/jdk/pull/11298/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11298&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297444 Stats: 3440 lines in 35 files changed: 1198 ins; 2212 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/11298.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11298/head:pull/11298 PR: https://git.openjdk.org/jdk/pull/11298
