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

[Maurizio Cimadamore]

Good work, some comments:

* in the combo test API, who is calling returnTask()?

* would it make sense to use try with resources for JavacPoolTask and 
jshell AnalyzeTask, to minimize chances of errors?

* 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

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?

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...

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
>