RFR(xs): 8134744: WorkerThread::id() should match GangWorker's worker_id

[Jon Masamitsu]

On 09/01/2015 02:44 AM, Per Liden wrote:
> Hi Derek,
>
> On 2015-09-01 00:12, Derek White wrote:
>> Hi Per,
>>
>> I'm confused on the same issue, or I've gotten lost between thinking
>> about [Abstract] (WorkGang | GangWorker) :-)
>>
>> AbstractWorkGang::initialize_workers() assigns ids from
>> 0..total_workers(). This id is also GangWorker's index into the
>> AbstractWorkGang's_worker array. So changing GangWorker's id breaks
>> implied consistency that AbstractWorkGang::worker(some_gang_worker._id)
>> == some_gang_worker. Right? This seem more confusing than the WorkData's
>> _worker_id != GangerWorkers._id.
>
> When GangWorkers are created they are assigned id 0...total_workers - 
> 1, for the sole purpose of being able to create a unique string to 
> represent its name ("GCWorker#1", "GCWorker#2", etc). This id has no 
> true relation to how the WorkGang wants to organize the threads. I.e. 
> it's not really used as an index into WorkGang's private array of 
> GangWorkers.
>
>>
>> Note that I didn't see how WorkerThread::id(), mentioned below, fits in
>> this picture.
>
> GangWorker inherits from WorkerThread.
>
>>
>> Since the WorkData's _worker_id is arbitrarily handed out, but the
>> GangWorker's id is an index into an array, it seems safer to keep
>> GangWorker's ID static, and setting WorkData's id when it gets assigned
>> to a worker.
>
> That wouldn't work with how this is used today. When you execute 
> gang->run_task(task), the task::work(worker_id) function should be 
> called as many times as there are workers, with each worker_id being 
> unique.
>
> I think the fundamental problem here is that the concepts of "work id" 
> and "worker id" is kind of mixed here and it didn't really matter 
> before the recent changes to the GangWorker because they used to 
> always be the same. The recent changes does provide a really nice 
> optimization, but exposes this inconsistency.

Yes, "worker id" is often used as the "work id" and it would be good to 
stop doing that
but I think there is value in having a unique identifier for each 
worker.   A GC worker
does have an identify in the OS (which is just some pointer I assume) 
and having the
GC worker identify changing when the OS identifier is static might cause 
confusion.

Jon

>
> The alternative here would of course be to just leave this as is, but 
> I'm worried we might see other bugs like this.
>
> Do other people have opinions on this?
>
> cheers,
> /Per
>
>>
>>   - Derek
>>
>> On 8/31/15 2:35 PM, Jon Masamitsu wrote:
>>> Per,
>>>
>>> A GangWorker is given an "uint id" when it is
>>> created.  If it is going to be reset the id whenever it
>>> does work, should it have its own "id"?
>>>
>>> Jon
>>>
>>> On 08/31/2015 07:16 AM, Per Liden wrote:
>>>> Hi,
>>>>
>>>> This is a follow up patch to JDK-8134749 (SoftReferences declared
>>>> dead too early), which makes sure WorkerThread::id() and match
>>>> GangWorker's worker_id always match. This allows them to be used
>>>> interchangeably, which is both convenient and removes a potential
>>>> source for bugs (it's easy to assumed they are the same). This change
>>>> should not affect any existing code.
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~pliden/8134744/webrev.0/
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8134744
>>>>
>>>> Testing: jprt
>>>>
>>>> /Per
>>>>
>>>
>>