RFR: JDK-8186694: JShell: speed-up compilation by reusing compiler instances

Mon Oct 16 13:10:29 UTC 2017

Looks good

Maurizio


On 11/10/17 15:45, Jan Lahoda wrote:
> I've updated the patch to the consolidated repository and to the LVTI 
> changes, webrev is available here:
> http://cr.openjdk.java.net/~jlahoda/8186694/webrev.03/
>
> The only significant change should be use of TaskListener instead of a 
> subclass of com.sun.tools.javac.main.JavaCompiler to fix the 
> intersection types.
>
> Is this still OK?
>
> Thanks,
>     Jan
>
> On 1.9.2017 17:57, Maurizio Cimadamore wrote:
>> I like this - ship it!
>>
>> Thanks
>> Maurizio
>>
>>
>> On 01/09/17 13:21, Jan Lahoda wrote:
>>> Hi,
>>>
>>> Thanks. I've updated the patch so that the pool (and both JShell and
>>> tests) take a worker and the tasks are only valid while the worker is
>>> running:
>>>
>>> http://cr.openjdk.java.net/~jlahoda/8186694/webrev.02/
>>>
>>> What do you think?
>>>
>>> Thanks,
>>>     Jan
>>>
>>> On 31.8.2017 16:17, Maurizio Cimadamore wrote:
>>>> Looks better, thanks.
>>>>
>>>> On the issue of getTask/returnTask, I think that either a
>>>> lambda-accepting method or a TWR approach would be good in 
>>>> enforcing the
>>>> 'right' idiom. Another alternative would be to return a task which
>>>> overrides the parse()/analze()... methods so that the returnTask logic
>>>> is called at the end. But that seems weaker than the lambda based
>>>> approach.
>>>>
>>>> I'm not too worried w.r.t. inconsistencies with the 
>>>> JavaCompiler.getTask
>>>> because (i) even having a pair of methods is inconsistent with that 
>>>> API
>>>> and (ii) JavacPoolTask represents a pool of compilation tasks, so I'm
>>>> not sure it has to be 100% consistent with what's done in 
>>>> JavaCompiler.
>>>>
>>>> Updating the clients would probably be the biggest chunk of work, so I
>>>> leave with you as to whether this should be addressed. My sense is 
>>>> that
>>>> some of the jshell clients have already changes because of this (with
>>>> try/finally, so adding a method call with a lambda would probably be
>>>> rather similar). But combo tests would also need same treatment (e.g.
>>>> ComboInstance::newCompilationTask should probably become a
>>>> lambda-accepting ComboInstance::withCompilatonTask).
>>>>
>>>> Maurizio
>>>>
>>>>
>>>> On 31/08/17 10:14, Jan Lahoda wrote:
>>>>> Thanks for the comments so far.
>>>>>
>>>>> Updated webrev:
>>>>> http://cr.openjdk.java.net/~jlahoda/8186694/webrev.01/
>>>>>
>>>>> I've moved the class to the javac.api package as suggested, and did
>>>>> more changes as noted below.
>>>>>
>>>>> One of the additional changes is a Scope related fix to TypeEnter and
>>>>> corresponding ScopeClassHeaderTest.java test, which is mostly
>>>>> independent to the rest of this patch, and I can split that out. 
>>>>> (Some
>>>>> of the fixes in this webrev caused this bug was revealed.)
>>>>>
>>>>> On 30.8.2017 12:43, Maurizio Cimadamore wrote:
>>>>>> Good work, some comments:
>>>>>>
>>>>>> * in the combo test API, who is calling returnTask()?
>>>>>
>>>>> Ooops. Fixed (when a new task is to be issued, the previous one is
>>>>> closed).
>>>>>
>>>>>>
>>>>>> * would it make sense to use try with resources for JavacPoolTask 
>>>>>> and
>>>>>> jshell AnalyzeTask, to minimize chances of errors?
>>>>>
>>>>> Makes sense, done.
>>>>>
>>>>>>
>>>>>> * not sure about the getTask/returnTask handshake - the logic 
>>>>>> seems to
>>>>>> work in this way:
>>>>>>
>>>>>> - when a new task is requested, you check the caches, and if a 
>>>>>> context
>>>>>> is there (with given options) you return it, otherwise you create a
>>>>>> fresh one
>>>>>> - when a task is returned, if it's not polluted, you add it to the
>>>>>> caches. Then you need to trim the caches to make sure there's 
>>>>>> room for
>>>>>> the new context
>>>>>
>>>>> Yes, that's the intent.
>>>>>
>>>>>>
>>>>>> We badly miss a circular queue here (not a fault of this patch of
>>>>>> course
>>>>>> :-)).
>>>>>>
>>>>>> If we could reduce the state to the options2Context map, then we 
>>>>>> could
>>>>>> probably use a CHM and get rid of synchronized block.
>>>>>>
>>>>>> Even w/ synchronized map, it seems like 'pool' is only used to 
>>>>>> get at
>>>>>> the first cached context, so maybe a simple field (which you
>>>>>> overwrite)
>>>>>> might suffice?
>>>>>
>>>>> I've got rid of the pool field. But not sure if the code can be
>>>>> rewritten to CHM - might be safer to simply use a simple synchronized
>>>>> block.
>>>>>
>>>>>>
>>>>>> More generally, I tend to view pair of coupled methods such as
>>>>>> getTask/returnTask as suspicious and error prone (in case somebody
>>>>>> forgets to call the second method) - so I tend to try to avoid 
>>>>>> writing
>>>>>> such APIs unless absolutely necessary - here I'm not 100% on why
>>>>>> this is
>>>>>> necessary... on the one hand I get the desire of knowing when a 
>>>>>> client
>>>>>> is done with a task, on the other hand there's nothing stopping the
>>>>>> same
>>>>>> client from keep using same task after returning it, so...
>>>>>
>>>>> In principle I agree having a pair of methods can lead to errors. But
>>>>> the error here is that a Context won't be reused, which just should
>>>>> mean things will be somewhat slower, but the results should be OK. I
>>>>> can see there are some options, but I am not sure if they are better:
>>>>> a) when asked for a task, we could simply reuse the last Context,
>>>>> without checking if the previous user is finished with that. (This is
>>>>> what the tests are doing now.) I think this was OK for tests, and
>>>>> might work for JShell as well. But if the previous user would be 
>>>>> still
>>>>> using the Context, the outcomes are hardly predictable (can lead to
>>>>> surprising broken results). So in JShell, each time we would create a
>>>>> new task, we would need to ask ourselves if there's another task
>>>>> currently in use. So this seems like complicating maintenance.
>>>>>
>>>>> b) make the tasks only available inside a worker method. Like:
>>>>> <Z> Z getTask(<getTask params>, Function<JavacTask, Z> worker) {
>>>>> ...
>>>>> }
>>>>>
>>>>> the task is only valid while "worker" is running. This is safer, as
>>>>> one cannot forget to close the task. (And I rewrote JShell to do this
>>>>> as an experiment.) But not sure if this is not too complex as this
>>>>> means significant changes to the callers, and is also inconsistent
>>>>> with JavaCompiler.getTask.
>>>>>
>>>>> Jan
>>>>>
>>>>>>
>>>>>> Maurizio
>>>>>>
>>>>>>
>>>>>> On 29/08/17 21:11, Jonathan Gibbons wrote:
>>>>>>> I don't think JavacTaskPool belongs in the javac.util package.
>>>>>>>
>>>>>>> Unless there are good reasons for doing otherwise, how about using
>>>>>>> javac.api instead ... i..e closed to JavacTaskImpl and friend.
>>>>>>>
>>>>>>> -- Jon
>>>>>>>
>>>>>>>
>>>>>>> On 08/29/2017 12:58 PM, Jan Lahoda wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> When starting jshell like this (where "/tmp/script" is a file 
>>>>>>>> which
>>>>>>>> has just "/exit"):
>>>>>>>> $ jshell PRINTING --execution local /tmp/script
>>>>>>>>
>>>>>>>> It currently takes approx. 5-6s on my laptop. PRINTING will add
>>>>>>>> quite
>>>>>>>> a few startup snippets. Each of these snippets is processed
>>>>>>>> separately and needs several instances of javac to be created 
>>>>>>>> to be
>>>>>>>> processed correctly.
>>>>>>>>
>>>>>>>> So, the idea in this patch is to reuse the javac instances 
>>>>>>>> multiple
>>>>>>>> (many) times. This avoids initializing the javac instances just to
>>>>>>>> process a very small ("single line") input.
>>>>>>>>
>>>>>>>> This patch is based on the ReusableContext by Maurizio used in
>>>>>>>> tests.
>>>>>>>> The ReusableContext handling is wrapped in JavacTaskPool, which
>>>>>>>> keeps
>>>>>>>> the ReusableContext and issues JavacTask possibly based on a 
>>>>>>>> reused
>>>>>>>> Context. The pool is not currently general purpose, but should 
>>>>>>>> work
>>>>>>>> for tests for which ReusableContext worked and for JShell.
>>>>>>>>
>>>>>>>> One tricky thing is that in JShell, it is necessary to prune
>>>>>>>> everything in the "REPL" package when reusing the Context, as 
>>>>>>>> it is
>>>>>>>> necessary to reload the correct/updated classes again from the 
>>>>>>>> file
>>>>>>>> manager (this is achieved by additional cleaning in TaskFactory).
>>>>>>>>
>>>>>>>> Overall, with this patch, the time to execute the above command is
>>>>>>>> approx. 2.5s
>>>>>>>>
>>>>>>>> Bug:
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8186694
>>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~jlahoda/8186694/webrev.00/
>>>>>>>>
>>>>>>>> How does this look?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>     Jan
>>>>>>>
>>>>>>
>>>>
>>