RFR (XS): 8033443: Test8000311 fails after latest changes to parallelize string and symbol table unlink

[Bengt Rutisson]

Hi Thomas,

On 2/5/14 12:21 PM, Thomas Schatzl wrote:
> Hi,
>
> On Tue, 2014-02-04 at 17:48 +0100, Bengt Rutisson wrote:
>> Hi Thomas,
>>
>> This looks good to me.
>>
>> One minor thing that I leave up to you to fix if you want to.
>>
> I would like to keep it for now - see below.

I agree with you. More comments below, but just to be clear I am fine 
with your change, consider it reviewed.

>
>> It seems more natural to me to pass an "is parallel" value to the
>> constructor of G1StringSymbolTableUnlinkTask rather than have it look
>> at G1CollectedHeap::use_parallel_gc_threads() itself to set the
>> _do_in_parallel value.
> Then you did not look too closely on implementations of AbstractGangTask
> yet ;)
>
> Typical pattern:
>
> MyTask x(...);
>
> void x::work() {
>    if (G1CollectedHeap::use_parallel_gc_threads()) {
>      ...
>    } else {
>      ...
>    }
> }
>
> num_workers =
> calculate_number_of_workers(G1CollectedHeap::use_parallel_gc_threads())
>
> if (G1CollectedHeap::use_parallel_gc_threads()) {
>     g1h->set_par_threads();  //
>     workers()->run_task(&g1_par_task);
>     g1h->set_par_threads(0); //
> } else {
>     g1_par_task.set_for_termination(n_workers); //
>     g1_par_task.work(0);
> }
>
> (Line marked with // are purely optional it seems ;) (they are not - and
> sometimes you need another call to workgang()->set_n_workers() or so).
>
> I think it would be more natural that the AbstractGangTask provided this
> information in whatever form instead of the programmer taking care of
> that every time...
>
> I.e. either there are different methods to override (e.g. the parallel
> one just defaults to the serial one), or the task dispatch sets a flag
> that can be queried.
>
> Also there could be some method that contains the boiler-plate code,
> i.e. given
>
> MyTask x(...);
> num_workers = ...
>
> [bool ran_in_parallel =] possibly_run_parallel(AbstractGangTask* task,
> uint num_workers);
>
> (the bool result because some code needs to know whether we actually
> performed the code in parallel for eg. asserts).
>
> where possibly_run_parallel() or whatever is is called looks as follows:
>
> bool G1CollectedHeap::possibly_run_parallel(AbstractGangTask* task, uint
> num_workers) {
> if (num_workers > 1) {
>     [...]
>     workers()->run_task(&task);
>     [...]
> } else {
>     g1_par_task.work(0);
> }
>
> What do you think?

You are right, I didn't go and look at the AbstractGangTask 
implementation and usages. Now that you bring it up I realize that this 
is something that has been disturbing me a long time.

I think your suggestions sound good. It would be very nice if we can 
find a way to get rid of the duplicated code to set the workers up.

Thanks,
Bengt


>
> Thomas
>
>