ForkJoinTask does not re-check task status entailing overhead

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

ForkJoinTask does not re-check task status entailing overhead

JSR166 Concurrency mailing list
Hi,

In one of our test we create around 260,000 ForkJoinTasks from one parent task. The parent task sets a flag which causes every child task to cancel itself (using super.cancel).

Please find attached a class to reproduce the test. In Java 8, this test is instantaneous, whereas it takes around 25s on my machine with Java 11.

After investigation, it seems that this difference in duration comes from a change in the ForkJoinPool#awaitJoin(WorkQueue, ForkJoinTask<?>, long) function, which does not check that the task status is DONE anymore:

* in Java 8, before trying to help or removeAndExec, we check the current task status:
for (;;) {
    if ((s = task.status) < 0)
        break;
    // else, try to help other tasks
}
As the task was cancelled, the function returns immediately.

* in Java 11, the check is not performed, which causes the execution of the ForkJoinPool#tryRemoveAndExec function, which vainly browses the list of tasks and induces the overhead.

The question is: is the check on the task status necessary, or did we do something wrong?

Thanks,
Rémi Barat


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

InterruptedForkJoinTasks.java (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ForkJoinTask does not re-check task status entailing overhead

JSR166 Concurrency mailing list
On 11/6/18 4:11 AM, Rémi Barat via Concurrency-interest wrote:
> Hi,
>
> In one of our test we create around 260,000 ForkJoinTasks from one
> parent task. The parent task sets a flag which causes every child task
> to cancel itself (using super.cancel).
>
> Please find attached a class to reproduce the test. In Java 8, this test
> is instantaneous, whereas it takes around 25s on my machine with Java 11.
> ...

> The question is: is the check on the task status necessary, or did we do
> something wrong?

The check is not necessary assuming that exec() returns true if done
(either normally or exceptionally), which it is required to do. You
should change the line:

        @Override
        protected boolean exec() {
            if (!interrupted.get()) {
                executeTask();
                return true;
            } else {
                super.cancel(false);
                return true; // was false
            }
        }

(In jdk8, it happened to work OK even without doing this.)
You could also add a line in your checkInterrupt method:

        protected void checkInterruption() {
            if (interrupted.get()) {
                cancel(false);    // added
                throw new CancellationException();
            }
        }


-Doug


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ForkJoinTask does not re-check task status entailing overhead

JSR166 Concurrency mailing list
Hi, Doug,

Thank you very much for your quick answer. We originally made exec() return false since the javadoc says "Returns: true if this task is known to have completed normally". Maybe it should be updated so that it matches your answer, that "... assuming that exec() returns true if done (either normally or exceptionally)".

Do you see any potential issue we might have after changing this return value from false to true?
It looks like it will simply set the DONE flag in the status through setDone in doExec, which should be idempotent with the already recorded abnormal completion of the super.cancel call.
However I'm not 100% sure of this, and we're using ForkJoinTasks and CountedCompleters quite extensively in our software. Hopefully our test suite will cover this! 

Rémi

On Tue, Nov 6, 2018 at 1:12 PM Doug Lea via Concurrency-interest <[hidden email]> wrote:
On 11/6/18 4:11 AM, Rémi Barat via Concurrency-interest wrote:
> Hi,
>
> In one of our test we create around 260,000 ForkJoinTasks from one
> parent task. The parent task sets a flag which causes every child task
> to cancel itself (using super.cancel).
>
> Please find attached a class to reproduce the test. In Java 8, this test
> is instantaneous, whereas it takes around 25s on my machine with Java 11.
> ...

> The question is: is the check on the task status necessary, or did we do
> something wrong?

The check is not necessary assuming that exec() returns true if done
(either normally or exceptionally), which it is required to do. You
should change the line:

        @Override
        protected boolean exec() {
            if (!interrupted.get()) {
                executeTask();
                return true;
            } else {
                super.cancel(false);
                return true; // was false
            }
        }

(In jdk8, it happened to work OK even without doing this.)
You could also add a line in your checkInterrupt method:

        protected void checkInterruption() {
            if (interrupted.get()) {
                cancel(false);    // added
                throw new CancellationException();
            }
        }


-Doug


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ForkJoinTask does not re-check task status entailing overhead

JSR166 Concurrency mailing list
On 11/6/18 9:50 AM, Rémi Barat via Concurrency-interest wrote:
> Hi, Doug,
>
> Thank you very much for your quick answer. We originally made exec()
> return false since the javadoc says "Returns: true if this task is known
> to have completed /normally/". Maybe it should be updated so that it
> matches your answer, that "... assuming that exec() returns true if done
> (either normally or exceptionally)".

Yes; thanks -- "normally" should/will be deleted in that sentence
(especially since it does not mesh with the following sentence
clarification.) Sorry for the confusion!

>
> Do you see any potential issue we might have after changing this return
> value from false to true?

No.

-Doug


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: ForkJoinTask does not re-check task status entailing overhead

JSR166 Concurrency mailing list
Hi, Doug,

Thank you for the clarification and for your time. It seems all clear on our side when setting the return value to true.

Have a good day,
Rémi

On Tue, Nov 6, 2018 at 5:39 PM Doug Lea via Concurrency-interest <[hidden email]> wrote:
On 11/6/18 9:50 AM, Rémi Barat via Concurrency-interest wrote:
> Hi, Doug,
>
> Thank you very much for your quick answer. We originally made exec()
> return false since the javadoc says "Returns: true if this task is known
> to have completed /normally/". Maybe it should be updated so that it
> matches your answer, that "... assuming that exec() returns true if done
> (either normally or exceptionally)".

Yes; thanks -- "normally" should/will be deleted in that sentence
(especially since it does not mesh with the following sentence
clarification.) Sorry for the confusion!

>
> Do you see any potential issue we might have after changing this return
> value from false to true?

No.

-Doug


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest