Re: RFR: jsr166 jdk integration 2018-02

[Martin Buchholz]

Hans, thanks for another great lesson!

On Fri, Feb 9, 2018 at 11:19 AM, Hans Boehm  wrote:

>
> The downside of treating "this" specially is that it makes it even harder
> to explain the other cases, e.g. when the object being finalized
> prematurely was an explicit parameter, or perhaps even the result of a
> factory method.
>

Java programmer intuition says the "this" object receiver is special.  E.g.
there's no way to explicitly null out "this" in a method body to be nice to
the GC, as you can with a method argument.   Think how crazy "this = null;"
looks! But that's what the VM is doing on your behalf.

Anyways, back in the real world this change got submitted with
reachabilityFence.

Re: RFR: jsr166 jdk integration 2018-02

[Paul Sandoz]

+1

Hans is right that reachabilityFence can be clumsy but it’s the best we have 
right now.
 
Paul.

> On Feb 8, 2018, at 5:19 PM, Martin Buchholz  wrote:
> 
> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html
>  
> 
> 
> 8190324: ThreadPoolExecutor should not specify a dependency on finalization
> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ThreadPoolExecutor-finalize/index.html
>  
> 
> https://bugs.openjdk.java.net/browse/JDK-8190324 
> 
> 
> Another set of uninteresting changes, some suggested by intellij:
> 
> 8195590: Miscellaneous changes imported from jsr166 CVS 2018-02
> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html
>  
> 
> https://bugs.openjdk.java.net/browse/JDK-8195590 
> 
> 

Re: RFR: jsr166 jdk integration 2018-02

[Martin Buchholz]

On Thu, Feb 8, 2018 at 10:39 PM, David Holmes 
wrote:

>
> Well at least it's only an issue if there does exist a finalize() method
> that could interfere with the executing method. I have to assume that
> somehow the VM is clever enough that if 'this' is in a register and we will
> later access a field at this+offset, that the GC will not free the memory
> used by 'this'.
>

I expect the opposite, that the VM may see an opportunity to prefetch that
other field.  Maybe it needed to read an adjacent field, and was able to
read both fields with a single instruction.  Maybe escape analysis allowed
it to infer that the object could never be accessed by another thread that
might write to the field.

Even more extreme - in the case of
 newSingleThreadExecutor().execute(someTask);
the VM can do enough analysis to conclude the executor will never be used
after invoking super.execute, so the VM can "optimize" by constructing the
executor, immediately finalizing it, and only then calling the body of
execute.

Re: RFR: jsr166 jdk integration 2018-02

[David Holmes]

On 9/02/2018 3:35 PM, Martin Buchholz wrote:
On Thu, Feb 8, 2018 at 8:39 PM, David Holmes > wrote:

Wow! DelegatedExecutorService doesn't even have a finalize() method
that does a shutdown. So we have to put the reachabilityFence in it
to prevent the delegatee from becoming unreachable due to the
delegator becoming unreachable!

This whole notion that "this" can be unreachable whilst a method on
it is still executing is just broken - it's an unusable programming
model. Any code that does "new Foo().bar()" could fail unless
Foo.bar() has a reachability fence in it. :( Sheesh!

I've argued similarly even in the past month that "this" should always 
remain reachable while a method on "this" is executing, in the presence 
of a finalize method.  (It would also be nice if java language scope 
mapped to reachability range, but that information is already lost in 
the translation to bytecode.)  But apparently it is quite a burden on 
any implementation to ensure reachability, especially after inlining.


We've never had a report of this problem occurring in practice.


Well at least it's only an issue if there does exist a finalize() method 
that could interfere with the executing method. I have to assume that 
somehow the VM is clever enough that if 'this' is in a register and we 
will later access a field at this+offset, that the GC will not free the 
memory used by 'this'.


Somewhat ironic we add the reachabilityFence in the same changeset that 
removes the action of the finalize() method  in TPE. (But of course the 
ExecutorService passed to the delegating service may still have one.)


David

Re: RFR: jsr166 jdk integration 2018-02

[Martin Buchholz]

On Thu, Feb 8, 2018 at 8:39 PM, David Holmes 
wrote:

>
> Wow! DelegatedExecutorService doesn't even have a finalize() method that
> does a shutdown. So we have to put the reachabilityFence in it to prevent
> the delegatee from becoming unreachable due to the delegator becoming
> unreachable!
>
> This whole notion that "this" can be unreachable whilst a method on it is
> still executing is just broken - it's an unusable programming model. Any
> code that does "new Foo().bar()" could fail unless Foo.bar() has a
> reachability fence in it. :( Sheesh!
>

I've argued similarly even in the past month that "this" should always
remain reachable while a method on "this" is executing, in the presence of
a finalize method.  (It would also be nice if java language scope mapped to
reachability range, but that information is already lost in the translation
to bytecode.)  But apparently it is quite a burden on any implementation to
ensure reachability, especially after inlining.

We've never had a report of this problem occurring in practice.

Re: RFR: jsr166 jdk integration 2018-02

[David Holmes]

Hi Martin,

On 9/02/2018 2:07 PM, Martin Buchholz wrote:



On Thu, Feb 8, 2018 at 7:39 PM, David Holmes > wrote:


On 9/02/2018 11:19 AM, Martin Buchholz wrote:


http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html



8190324: ThreadPoolExecutor should not specify a dependency on
finalization

http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ThreadPoolExecutor-finalize/index.html


https://bugs.openjdk.java.net/browse/JDK-8190324



Looks okay.

Another set of uninteresting changes, some suggested by intellij:

8195590: Miscellaneous changes imported from jsr166 CVS 2018-02

http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html


https://bugs.openjdk.java.net/browse/JDK-8195590



H ... a summary of changes in the bug report would be nice.


Done.

I don't find the addition of reachability fences uninteresting
though. Why are they needed in those specific cases?


perhaps the standard should be for every method of a class with a 
finalizer to have reachabilityFence(this) at the end?


Imagine

newSingleThreadExecutor().execute(someTask);

Is is unlikely but possible for the "instant garbage" executor to be 
finalized while super.execute is in progress, causing failure with 
RejectedExecutionException because the delegatee is already shutdown.


Wow! DelegatedExecutorService doesn't even have a finalize() method that 
does a shutdown. So we have to put the reachabilityFence in it to 
prevent the delegatee from becoming unreachable due to the delegator 
becoming unreachable!


This whole notion that "this" can be unreachable whilst a method on it 
is still executing is just broken - it's an unusable programming model. 
Any code that does "new Foo().bar()" could fail unless Foo.bar() has a 
reachability fence in it. :( Sheesh!


Cheers,
David

Re: RFR: jsr166 jdk integration 2018-02

[Martin Buchholz]

On Thu, Feb 8, 2018 at 7:39 PM, David Holmes 
wrote:

> On 9/02/2018 11:19 AM, Martin Buchholz wrote:
>
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integr
>> ation/overview.html
>>
>> 8190324: ThreadPoolExecutor should not specify a dependency on
>> finalization
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integr
>> ation/ThreadPoolExecutor-finalize/index.html
>> https://bugs.openjdk.java.net/browse/JDK-8190324
>>
>
> Looks okay.
>
> Another set of uninteresting changes, some suggested by intellij:
>>
>> 8195590: Miscellaneous changes imported from jsr166 CVS 2018-02
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integr
>> ation/miscellaneous/index.html
>> https://bugs.openjdk.java.net/browse/JDK-8195590
>>
>
> H ... a summary of changes in the bug report would be nice.
>
>
Done.


> I don't find the addition of reachability fences uninteresting though. Why
> are they needed in those specific cases?
>

perhaps the standard should be for every method of a class with a finalizer
to have reachabilityFence(this) at the end?

Imagine

newSingleThreadExecutor().execute(someTask);

Is is unlikely but possible for the "instant garbage" executor to be
finalized while super.execute is in progress, causing failure with
RejectedExecutionException because the delegatee is already shutdown.

Re: RFR: jsr166 jdk integration 2018-02

[David Holmes]

On 9/02/2018 11:19 AM, Martin Buchholz wrote:

http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html

8190324: ThreadPoolExecutor should not specify a dependency on finalization
http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ThreadPoolExecutor-finalize/index.html
https://bugs.openjdk.java.net/browse/JDK-8190324


Looks okay.


Another set of uninteresting changes, some suggested by intellij:

8195590: Miscellaneous changes imported from jsr166 CVS 2018-02
http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html
https://bugs.openjdk.java.net/browse/JDK-8195590


H ... a summary of changes in the bug report would be nice.

I don't find the addition of reachability fences uninteresting though. 
Why are they needed in those specific cases?


Thanks,
David