RFR JDK-8194978: Javac produces dead code for try-with-resource
Jan Lahoda
jan.lahoda at oracle.com
Thu Mar 22 14:35:24 UTC 2018
On 21.3.2018 12:02, Maurizio Cimadamore wrote:
> 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).
Thanks! I've added the comment and pushed:
http://hg.openjdk.java.net/jdk/jdk/rev/c2a3a2aa2475
Jan
>
> 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