RFR JDK-8194978: Javac produces dead code for try-with-resource
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Wed Mar 21 11:02:48 UTC 2018
Looks great - please add a comment (no need for new webrev) in
GenFInalizer::afterBody to explain why we are setting
tryEnv.info.finalize to null (e.g. to disable Gen::genFinalizer).
Maurizio
On 21/03/18 09:12, Jan Lahoda wrote:
> Hi Maurizio,
>
> Thanks a lot for the comments. I've updated the patch according to the
> comments, updated webrev is here:
> http://cr.openjdk.java.net/~jlahoda/8194978/webrev.01/
>
> How does that look?
>
> Thanks,
> Jan
>
> On 20.3.2018 13:13, Maurizio Cimadamore wrote:
>> Hi Jan,
>> great work. The desugaring looks correct, at least there's nothing wrong
>> I can spot about it. Some comments:
>>
>> * resourceNonNull = TreeInfo.skipParens(resource).hasTag(NEWCLASS);
>>
>> Is the above needed? I can't seem to support evidence that this syntax
>> is even legal (in the branch where 'resource' is an expression, I mean)
>>
>> * The javadoc in Lower::makeTwrTry seems out of sync what is being
>> actually generated
>>
>> * I think the Lower code would benefit from better variable names - the
>> patch has a mixture of old names (e.g. primaryException) and new names
>> (e.g. 'closeStatement2'). I suggest picking a naming scheme in the
>> javadoc, and then stick to that in the code, so that there are explicit
>> references to what bit it is that we're currently generating.
>>
>> * I find the code in Gen bit subtle; you want the new finally block to
>> act as a finally - but only during the try block; that means, I believe,
>> that you want genFinalizer to do the usual thing (e.g. copy finalizer
>> body on exit points) but then skip doing that when it comes to
>> exceptions. That is, you don't want a catch-all block and you don't want
>> to copy finalizer body on exceptional paths. Another stacking would be
>> to completely replace the 'env.info.finalize' after the body is done,
>> e.g.
>>
>> <visit try body>
>>
>> if (tree.finalizer.flags & BODY_ONLY_FINALIZE != 0) {
>> env.info.finalize == null;
>> }
>>
>> Wouldn't this be more consistent with what you are trying to achieve?
>> Seems to me that with the current approach there are side effects that
>> occur in the GenFinalizer even when afterBody is set (e.g. setting
>> finalizer gaps in the encl environment).
>>
>> Cheers
>> Maurizio
>>
>> On 15/03/18 15:40, Jan Lahoda wrote:
>>> Hi,
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8194978
>>>
>>> The issue here is that the bytecode generated for try-with-resources
>>> produces some unreachable instructions, and may be unnecessarily big.
>>>
>>> Consider this code:
>>> ---
>>> try (InputStream in = new FileInputStream(str)) {
>>> int read = in.read();
>>> if (read == (-1)) return ;
>>> System.err.println(read);
>>> }
>>> ---
>>>
>>> javac desugars it to this:
>>> ---
>>> {
>>> final InputStream in = new FileInputStream(str);
>>> /*synthetic*/ Throwable primaryException0$ = null;
>>> try {
>>> int read = in.read();
>>> if (read == (-1)) return;
>>> System.err.println(read);
>>> } catch (/*synthetic*/ final Throwable t$) {
>>> primaryException0$ = t$;
>>> throw t$;
>>> } finally {
>>> if (primaryException0$ != null) try {
>>> in.close();
>>> } catch (/*synthetic*/ Throwable x2) {
>>> primaryException0$.addSuppressed(x2);
>>> } else in.close();
>>> }
>>> }
>>> ---
>>>
>>> Which seems fine, but when the try-finally construct is written into
>>> bytecode, the "finally" section is duplicated into all
>>> (non-exceptional) exit points from the try body (it would also be
>>> copied to non-exception exit points from all catches, but there are no
>>> such exit points in this case). A rough idea on how the bytecode looks
>>> like is below - please note that some try-catches don't embed nicely
>>> with the rest of the code, so are denoted in comments only.
>>>
>>> ---
>>> {
>>> final InputStream in = new FileInputStream(str);
>>> /*synthetic*/ Throwable primaryException0$ = null;
>>> // try
>>> int read = in.read();
>>> if (read == (-1)) {
>>> // catch Throwable: goto CATCH
>>> // catch any: goto FINALLY [1]
>>> //start injected finally
>>> if (primaryException0$ != null) try { // [2]
>>> in.close();
>>> } catch (/*synthetic*/ Throwable x2) {
>>> primaryException0$.addSuppressed(x2);
>>> } else in.close();
>>> //end injected finally
>>>
>>> return;
>>> }
>>>
>>> // try
>>> System.err.println(read);
>>> // catch Throwable: goto CATCH
>>> // catch any: goto FINALLY [1]
>>>
>>> //start injected finally
>>> if (primaryException0$ != null) try { // [2]
>>> in.close();
>>> } catch (/*synthetic*/ Throwable x2) {
>>> primaryException0$.addSuppressed(x2);
>>> } else in.close();
>>> //end injected finally
>>>
>>> //goto END
>>>
>>> CATCH: // catch (/*synthetic*/ final Throwable t$)
>>> // try
>>> primaryException0$ = t$;
>>> throw t$;
>>> // catch any: goto FINALLY
>>> FINALLY:
>>> if (primaryException0$ != null) try {// [3]
>>> in.close();
>>> } catch (/*synthetic*/ Throwable x2) {
>>> primaryException0$.addSuppressed(x2);
>>> } else in.close();
>>> END:
>>> }
>>> ---
>>>
>>> A few comments about the code:
>>> -[1]: this catches seem unnecessary: all Throwables have already been
>>> caught?
>>> -[2]: at this place, it is clear that primaryException0$ is null, so
>>> the is-non-null section is not useful
>>> -[3]: at this place, it is mostly clear that primaryException0$ is
>>> not-null, so the is-null section is not useful
>>>
>>> So, it seems it might be possible to construct the code so that only
>>> the needed sections of the finally would be used at the appropriate
>>> places. I.e. the original try-with-resources would get desugared to
>>> something like:
>>>
>>> ---
>>> {
>>> final InputStream in = new FileInputStream(str);
>>> try {
>>> int read = in.read();
>>> if (read == (-1)) return;
>>> System.err.println(read);
>>> } body-only-finally { //copied to exit points in try body
>>> //only, not used as a real finally
>>> in.close();
>>> } catch (/*synthetic*/ final Throwable t$) {
>>> try {
>>> in.close();
>>> } catch (/*synthetic*/ Throwable x2) {
>>> t$.addSuppressed(x2);
>>> }
>>> throw t$;
>>> }
>>> }
>>> ---
>>>
>>> Which would then get generated to bytecode as:
>>> ---
>>> {
>>> final InputStream in = new FileInputStream(str);
>>> //try
>>> int read = in.read();
>>> if (read == (-1)) {
>>> // catch Throwable: goto CATCH
>>> in.close();
>>> return;
>>> }
>>>
>>> //try
>>> System.err.println(read);
>>> // catch Throwable: goto CATCH
>>>
>>> in.close();
>>>
>>> //goto END
>>>
>>> CATCH: // catch (/*synthetic*/ final Throwable t$)
>>> try {
>>> in.close();
>>> } catch (/*synthetic*/ Throwable x2) {
>>> t$.addSuppressed(x2);
>>> }
>>> throw t$;
>>> END:
>>> }
>>> ---
>>>
>>> The current desugaring is trying to pull out the finally code into a
>>> separate method when that appears to be worthwhile, but this proposed
>>> desugaring seems to be producing a better bytecode with less javac
>>> code.
>>>
>>> Does this updated desugaring/transformation look safe?
>>>
>>> Webrev: http://cr.openjdk.java.net/~jlahoda/8194978/webrev.00/
>>>
>>> Thanks,
>>> Jan
>>
More information about the compiler-dev
mailing list