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:38:07 UTC 2015
> > 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... :-)
>
> What I'd be most interested from you would be any interprocess and
synchronization stuff.
> Clashes between JDI threads and remote, risk of hangs, races, ...
This will take more mulling. I'll likely have more questions
about how JShell works (1 VM or 2 VMs or ...)
Dan
On 9/24/15 11:44 AM, Robert Field wrote:
>
> On 09/23/15 16:28, Daniel D. Daugherty wrote:
>> > http://cr.openjdk.java.net/~rfield/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.
>
> Jan will answer this.
>
>>
>> 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().
>
> That is indeed JDI ReferenceType being used to a JDI 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'.
>
> Comment added on here and match remote side.
>
>>
>> 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(). :-)
>
> ;-)
>
> Yep, that's why you are here
>
>
>>
>> L268: //could also tag the thread (e.g. using
>> name), to find it easier
>> Space after '//'.
>
> Added
>
>>
>> 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...
>
> Added:
>
> * Adapted from jdb VMConnection. Message handling, exception
> handling, and I/O
> * redirection changed. Interface to JShell added.
>
>>
>> L321: synchronized VirtualMachine open() {
>> <snip>
>> L332: if (vm.canBeModified()){
>> L333: // setEventRequests(vm);
>> L334: // resolveEventRequests();
>> L335: }
>>
>> Why commented out instead of deleted?
>
> Moved the test and calls into a new method, and made this:
>
> // Uncomment here and below to enable event requests
> // installEventRequests(vm);
>
> There are features that have been discussed that would require adding
> event requests
>
>>
>> L397: /***
>> L398: private void setEventRequests(VirtualMachine vm) {
>> <snip>
>> L419: private void resolveEventRequests() {
>> L420: Env.specList.resolveAll();
>> L421: }
>> L422: ***/
>>
>> Why commented out instead of deleted?
>
> /*** Preserved for possible future support of event requests
>
>>
>> L429: pStream.print((char)i);
>> L434: throw ex;
>> The indents are wrong for these two lines.
>
> Fixed
>
>>
>> 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...
>
> Yeah, that is in the stupid comment department ;-)
>
> and it is no longer definitely true, there has been considerable
> interest in
> features that would require this.
>
> Changed the comments to:
>
> /* JShell currently uses only launch, preserved for futures: */
>
>>
>> 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...
>
> Well, pretty much is extracted, Env is this big complex thing. I just
> pulled out
> four little methods. Changed the comment to:
>
> * Select methods extracted from jdb Env; shutdown() adapted to JShell
> shutdown.
>
> Just checked, the jdb code did not get removed, it is still sitting
> happily in:
>
> dev/jdk/src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty
>
>>
>> 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...
>
> * Adapted from jdb EventHandler; Handling of events not used by
> JShell stubbed out.
>
>>
>> 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...
>
> * Copy of jdb VMNotConnectedException.
>
>>
>> src/jdk.jshell/share/classes/jdk/jshell/JShell.java
>> L595 // --- priave / package-private implementation support ---
>> Typo? 'priave' Perhaps 'private'
>
> Fixed.
>
>>
>>
>> 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...
>
> Yes. Also, and most significantly to redefine classes holding the
> user's snippets of code as the user evolves them.
>
>> 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.
>
> Yes, we don't want to tread on the IDE/debugger space.
>
> I would like to add tracing at some point.
>
>>
>> There is some mention of RedefineClasses(), but I suspect that is
>> simply to redefine the code being evaluated so there's something
>> to run...
>
> If the user enters:
>
> int f(int x) { return x; }
>
> It is wrapped in a class, compiled, and just loaded.
>
> But if they then enter:
>
> int f(int x) { return x * x; }
>
> It is wrapped in a class (of the same name), compiled, and redefined
> with JDI RedefineClasses.
> (failing that, it does less graceful things)
>
> And that is why I'm using JDI.
>
>>
>> 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... :-)
>
> What I'd be most interested from you would be any interprocess and
> synchronization stuff.
> Clashes between JDI threads and remote, risk of hangs, races, ...
>
> Thanks,
> Robert
>
>>
>> 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>
>>> To: kulla-dev <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
>>>
>>> http://cr.openjdk.java.net/~rfield/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/
>>>
>>> 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
>>>
>>> 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