Dan: JDK 9 RFR of JDK-8134254 JShell API/tool: REPL for Java into JDK9
Jan Lahoda
jan.lahoda at oracle.com
Fri Sep 25 09:49:19 UTC 2015
Hi Dan,
Thanks for the comments! I've resolved the RemoteAgent comments:
http://hg.openjdk.java.net/kulla/dev/langtools/rev/eb209acb2451
Please let me know if there are any issues with that.
Thanks!
Jan
On 24.9.2015 08:10, Robert Field wrote:
> Thanks Dan!
>
> Forwarding to Kulla.
>
> -Robert
>
>> On Sep 23, 2015, at 4:43 PM, Michel Trudeau <michel.trudeau at oracle.com> wrote:
>>
>> Thank you Dan for the code review. Very appreciated.
>>
>> Thanks,
>> Michel
>>
>>
>>
>>
>> Daniel D. Daugherty wrote:
>>>> http://cr.openjdk.java.net/~rfield/jshell_langtools_webrev_v0/ <http://cr.openjdk.java.net/%7Erfield/jshell_langtools_webrev_v0/>
>>>
>>> src/jdk.jshell/share/classes/jdk/internal/jshell/remote/RemoteAgent.java
>>> L212: //thrown by the main process via JDI:
>>> Space after '//'.
>>>
>>> L226: void clientCodeLeave() {
>>> <snip>
>>> L228: while (expectingStop);
>>> Tight loop here is not kind. A very short sleep would be nice
>>> if you expect 'expectingStop' to transition from 'true' ->
>>> 'false' at some point. If you don't expect this thread to ever
>>> return from this loop, then 'Thread.sleep(<some_large_value>)'
>>> is good.
>>>
>>> Update: looks like expectingStop is set to 'false' in a few
>>> places so short sleep would be nice. If you call Thread.sleep(0)
>>> then that typically translates into the shortest sleep on the
>>> platform.
>>>
>>> src/jdk.jshell/share/classes/jdk/jshell/Eval.java
>>> L564: // First try Redefine
>>> L565: Map<ReferenceType, byte[]> mp = new TreeMap<>();
>>> L566: List<ClassNameBytes> newNameBytesList = new ArrayList<>();
>>> L567: for (ClassNameBytes nb : nameBytesList) {
>>> L568: ReferenceType rt = state.executionControl().nameToRef(nb.className);
>>> L569: if (rt != null) {
>>> L570: mp.put(rt, nb.bytes);
>>> L571: } else {
>>> L572: newNameBytesList.add(nb);
>>> L573: }
>>> L574: }
>>>
>>> I'm not quite understanding this use of com.sun.jdi.ReferenceType,
>>> but I suspect that the mentions of 'Redefine' here are not
>>> related to JVM/TI RedefineClasses().
>>>
>>> src/jdk.jshell/share/classes/jdk/jshell/ExecutionControl.java
>>> L68: // out before in
>>> L69: out = new ObjectOutputStream(socket.getOutputStream());
>>> L70: in = new ObjectInputStream(socket.getInputStream());
>>>
>>> Yes, the comment on L68 is right, but would better if it
>>> stated _why_ we create 'out' before 'in'.
>>>
>>> L154: boolean commandRedefine(Map<ReferenceType, byte[]> mp) {
>>> L155: try {
>>> L156: env.vm().redefineClasses(mp);
>>>
>>> Maybe I was wrong in my Eval.java comment about it not being
>>> related to JVM/TI RedefineClasses(). :-)
>>>
>>> L268: //could also tag the thread (e.g. using name), to find it easier
>>> Space after '//'.
>>>
>>> src/jdk.jshell/share/classes/jdk/jshell/JDIConnection.java
>>> L47: * Adapted from jdb JDIConnection.
>>> What are the significant differences? Did the original code get
>>> deleted as part of 'jdb' removal?
>>>
>>> Perhaps a short summary of the differences...
>>>
>>> L321: synchronized VirtualMachine open() {
>>> <snip>
>>> L332: if (vm.canBeModified()){
>>> L333: // setEventRequests(vm);
>>> L334: // resolveEventRequests();
>>> L335: }
>>>
>>> Why commented out instead of deleted?
>>>
>>> L397: /***
>>> L398: private void setEventRequests(VirtualMachine vm) {
>>> <snip>
>>> L419: private void resolveEventRequests() {
>>> L420: Env.specList.resolveAll();
>>> L421: }
>>> L422: ***/
>>>
>>> Why commented out instead of deleted?
>>>
>>> L429: pStream.print((char)i);
>>> L434: throw ex;
>>> The indents are wrong for these two lines.
>>>
>>> L506: /* We should never do this: attach to running target vm */
>>> L507: private VirtualMachine attachTarget() {
>>> So delete the function or have it throw an exception
>>> if used or...
>>>
>>> L517: /* We should never do this: alisten for connection from target vm */
>>> L518: private VirtualMachine listenTarget() {
>>> So delete the function or have it throw an exception
>>> if used or...
>>>
>>> src/jdk.jshell/share/classes/jdk/jshell/JDIEnv.java
>>> L33: * Extracted from jdb Env.
>>> Not quite 'Extracted'. Perhaps 'Adapted'. What are the differences?
>>> Did the original code get deleted as part of 'jdb' removal?
>>>
>>> Perhaps a short summary of the differences...
>>>
>>> src/jdk.jshell/share/classes/jdk/jshell/JDIEventHandler.java
>>> L33: * Adapted from jdb EventHandler.
>>> What are the significant differences? Did the original code get
>>> deleted as part of 'jdb' removal?
>>>
>>> Perhaps a short summary of the differences...
>>>
>>> src/jdk.jshell/share/classes/jdk/jshell/JDINotConnectedException.java
>>> L30: * Adapted from JDI JDINotConnectedException.
>>>
>>> Did the original code get deleted as part of 'jdb' removal?
>>>
>>> Perhaps a short summary of the differences...
>>>
>>> src/jdk.jshell/share/classes/jdk/jshell/JShell.java
>>> L595 // --- priave / package-private implementation support ---
>>> Typo? 'priave' Perhaps 'private'
>>>
>>>
>>> So if I understand the code correctly, JDI only comes into play in
>>> the "Eval" portion of the "Read-Eval-Print Loop". It's used to evaluate
>>> the code, to stop a currently executing evaluation either in a normal
>>> way or in a regain control way... Looks like just the exception handling
>>> piece is there and there's no use of JDI debugging capability to try
>>> and debug the code being evaluated.
>>>
>>> There is some mention of RedefineClasses(), but I suspect that is
>>> simply to redefine the code being evaluated so there's something
>>> to run...
>>>
>>> Please let me know if I didn't review something that you needed
>>> me to review. Also let me know if I'm even close to understanding
>>> what you're using JDI for in this area... :-)
>>>
>>> Dan
>>>
>>>
>>>
>>> On 9/10/15 1:06 PM, Robert Field wrote:
>>>> Hi Dan,
>>>>
>>>> Long time again.
>>>>
>>>> I've been working on adding a Read-Eval-Print Loop Java tool. We are on the verge of pushing it into JDK9, it has had an API review but needs a code review. It is a lot of new code, so we are breaking the task into areas. We have reviewers for the tool (built on the API), and for the compilation related areas. But we need someone for the JDI / remote interaction areas. Not much experience on this team in JDI. I'm hoping you would be will to do the review of that area. Let me know and I'll point you at the appropriate bits.
>>>>
>>>> Thanks,
>>>> Robert
>>>>
>>>>
>>>>
>>>> -------- Original Message --------
>>>> Subject: JDK 9 RFR of JDK-8134254 JShell API/tool: REPL for Java into JDK9
>>>> Date: Fri, 21 Aug 2015 23:06:06 -0700
>>>> From: Robert Field <robert.field at oracle.com> <mailto:robert.field at oracle.com>
>>>> To: kulla-dev <kulla-dev at openjdk.java.net> <mailto:kulla-dev at openjdk.java.net>
>>>>
>>>> Please review:
>>>>
>>>> 8134254 JShell API/tool: REPL for Java into JDK9
>>>> https://bugs.openjdk.java.net/browse/JDK-8134254 <https://bugs.openjdk.java.net/browse/JDK-8134254>
>>>>
>>>> http://cr.openjdk.java.net/~rfield/jshell_langtools_webrev_v0/ <http://cr.openjdk.java.net/%7Erfield/jshell_langtools_webrev_v0/>
>>>>
>>>> This webrev is of all of the JShell changes in langtools, which is the addition of the source code and all the tests.
>>>> For context, please familiarize yourself with the API first:
>>>>
>>>> http://cr.openjdk.java.net/~rfield/doc/ <http://cr.openjdk.java.net/%7Erfield/doc/>
>>>>
>>>> Since the webrev is all adds, and the order and access is far from ideal, it is almost certainly easier to clone the kulla repo:
>>>>
>>>> http://hg.openjdk.java.net/kulla/dev <http://hg.openjdk.java.net/kulla/dev>
>>>>
>>>> This webrev is the contents of these langtools directories
>>>>
>>>> src/jdk.jshell
>>>> test/jdk/jshell
>>>>
>>>> added to jdk9/dev/langtools.
>>>>
>>>> JShell.java and JShellImpl.java in src/jdk.jshell/share/classes/jdk/jshell/ would make a logical starting point for understanding the code.
>>>>
>>>> There are also small changes to the base jdk9 repo for make files and to add the module, and to the jdk repo to add the launcher, I will send these separately.
>>>>
>>>> Thanks,
>>>> Robert
>>>>
>>>>
>>>>
>>>
>>
>
More information about the kulla-dev
mailing list