Dan: JDK 9 RFR of JDK-8134254 JShell API/tool: REPL for Java into JDK9
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Sep 25 13:35:34 UTC 2015
Thanks for the update. Looks good.
Dan
On 9/25/15 3:49 AM, Jan Lahoda wrote:
> 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