[11]RFR: 8203030: Zero s390 31 bit size_t type conflicts in shared code

Wed Jun 20 09:38:57 UTC 2018

Hi
Another reviewer?

On 19/06/18 10:58 AM, "Chris Phillips"@T O wrote:
> Hi Per,
> Thanks!
> 
> On 19/06/18 10:41 AM, Per Liden wrote:
>> Hi Chris,
>>
>> On 06/19/2018 03:49 PM, Chris Phillips wrote:
>>> Hi Per
>>> Thanks!
>>> New webrev : http://cr.openjdk.java.net/~chrisphi/JDK-8203030/webrev.3
>>
>> Looks good to me!
>>
>> /Per
>>
>>> All suggested changes made (see below inline).
>>>
>>> On 18/06/18 05:55 AM, Per Liden wrote:
>>>> On 06/14/2018 05:01 PM, Chris Phillips wrote:
>>>>> Hi
>>>>> Any further comments or changes?
>>>>> On 06/06/18 05:56 PM, Chris Phillips wrote:
>>>>>> Hi Per,
>>>>>>
>>>>>> On 06/06/18 05:48 PM, Per Liden wrote:
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>> On 06/06/2018 11:15 PM, Chris Phillips wrote:
>>>>>>>> Hi Per,
>>>>>>>>
>>>>>>>> On 06/06/18 04:47 PM, Per Liden wrote:
>>>>>>>>> Hi Chris,
>>>>>>>>>
>>>>>>>>> On 06/06/2018 09:36 PM, Chris Phillips wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 06/06/18 02:23 PM, Per Liden wrote:
>>>>>>>>>>> On 2018-06-06 18:29, Andrew Haley wrote:
>>>>>>>>>>>> On 06/06/2018 04:47 PM, Chris Phillips wrote:
>>>>>>>>>>>>> Please review this set of changes to shared code
>>>>>>>>>>>>> related to S390 (31bit) Zero self-build type mis-match
>>>>>>>>>>>>> failures.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203030
>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~chrisphi/JDK-8203030/webrev.0
>>>>>>>>>>>>
>>>>>>>>>>>> Can you explain this a little more?  What is the type of
>>>>>>>>>>>> size_t on
>>>>>>>>>>>> s390x?  What is the type of uintptr_t?  What are the errors?
>>>>>>>>>>>
>>>>>>>>>>> I would like to understand this too.
>>>>>>>>>>>
>>>>>>>>>>> cheers,
>>>>>>>>>>> Per
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> Quoting from the original bug  review request:
>>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-June/014254.html
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> "This
>>>>>>>>>> is a problem when one parameter is of size_t type and the
>>>>>>>>>> second of
>>>>>>>>>> uintx type and the platform has size_t defined as eg. unsigned
>>>>>>>>>> long as
>>>>>>>>>> on s390 (32-bit)."
>>>>>>>>>
>>>>>>>>> Please clarify what the sizes of uintx (i.e. uintptr_t) and size_t
>>>>>>>>> are
>>>>>>>>> on s390?
>>>>>>>> See Dan's explanation.
>>>>>>>>>
>>>>>>>>> I fail to see how any of this matters to _entries here? What am I
>>>>>>>>> missing?
>>>>>>>>>
>>>>>>>>
>>>>>>>> By changing the type, to its actual usage, we avoid the
>>>>>>>> necessity of patching in
>>>>>>>> src/hotspot/share/gc/g1/g1StringDedupTable.cpp
>>>>>>>> around line 617, since its consistent usage and local I patched
>>>>>>>> at the
>>>>>>>> definition.
>>>>>>>>
>>>>>>>> - _table->_entries, percent_of(_table->_entries, _table->_size),
>>>>>>>> _entry_cache->size(), _entries_added, _entries_removed);
>>>>>>>> + _table->_entries, percent_of( (size_t)(_table->_entries),
>>>>>>>> _table->_size), _entry_cache->size(), _entries_added,
>>>>>>>> _entries_removed);
>>>>>>>>
>>>>>>>> percent_of will complain about types otherwise.
>>>>>>>
>>>>>>> Ok, so why don't you just cast it in the call to percent_of? Your
>>>>>>> current patch has ripple effects that you fail to take into account.
>>>>>>> For
>>>>>>> example, _entries is still printed using UINTX_FORMAT and compared
>>>>>>> against other uintx variables. You're now mixing types in an unsound
>>>>>>> way.
>>>>>>
>>>>>> Hmm missed that, so will do the cast instead as you suggest.
>>>>>> (Fixing at the defn is what was suggested the last time around so I
>>>>>> tried to do that where it was consistent, obviously this is not.
>>>>>> Thanks.
>>>>>>
>>>>>>> cheers,
>>>>>>> Per
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> src/hotspot/share/gc/g1/g1StringDedupTable.hpp
>>>>>>>>> @@ -120,11 +120,11 @@
>>>>>>>>>       // Cache for reuse and fast alloc/free of table entries.
>>>>>>>>>       static G1StringDedupEntryCache* _entry_cache;
>>>>>>>>>
>>>>>>>>>       G1StringDedupEntry**            _buckets;
>>>>>>>>>       size_t                          _size;
>>>>>>>>> -  uintx                           _entries;
>>>>>>>>> +  size_t                          _entries;
>>>>>>>>>       uintx _shrink_threshold;
>>>>>>>>>       uintx _grow_threshold;
>>>>>>>>>       bool _rehash_needed;
>>>>>>>>>
>>>>>>>>> cheers,
>>>>>>>>> Per
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hope that helps,
>>>>>>>>>> Chris
>>>>>>>>>>
>>>>>>>>>> (I'll answer further if needed but the info is in the bugs and
>>>>>>>>>> review thread mostly)
>>>>>>>>>> See:
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8203030
>>>>>>>>>> and:
>>>>>>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-June/014254.html
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8046938
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8074459
>>>>>>>>>> For more info.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> Cheers!
>>>>>> Chris
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Finally through testing and submit run again after Per's requested
>>>>> change, here's the knew webrev:
>>>>> http://cr.openjdk.java.net/~chrisphi/JDK-8203030/webrev.2
>>>>> attached is the passing run fron the submit queue.
>>>>>
>>>>> Please review...
>>>>
>>>>
>>>> src/hotspot/share/gc/cms/cms_globals.hpp
>>>> ----------------------------------------
>>>> Instead of changing the type of ParGCDesiredObjsFromOverflowList, I'd
>>>> suggest you just change the single place where you need a cast, in
>>>> ParScanThreadState::take_from_overflow_stack().
>>>>
>>>> If you change the type of ParGCDesiredObjsFromOverflowList, but you
>>>> otherwise have to clean up a number of places where it's already
>>>> explicitly cast to size_t in concurrentMaskSweepGeneration.cpp.
>>>>
>>> Done
>>>
>>>>
>>>> src/hotspot/share/gc/parallel/parallel_globals.hpp
>>>> --------------------------------------------------
>>>> Please also change to type of ParallelOldDeadWoodLimiterMean to size_t.
>>>>
>>>>
>>> Done
>>>
>>>> src/hotspot/share/gc/parallel/psParallelCompact.cpp
>>>> ---------------------------------------------------
>>>> No need to cast ParallelOldDeadWoodLimiterStdDev, you're already
>>>> changed
>>>> its type. And if you change ParallelOldDeadWoodLimiterMean to also
>>>> being
>>>> size_t you don't need to touch this file at all.
>>>>
>>>>
>>> Done
>>>
>>>> src/hotspot/share/runtime/globals.hpp
>>>> -------------------------------------
>>>> -define_pd_global(uintx,  InitialCodeCacheSize,       160*K);
>>>> -define_pd_global(uintx,  ReservedCodeCacheSize,      32*M);
>>>> +define_pd_global(size_t,  InitialCodeCacheSize,       160*K);
>>>> +define_pd_global(size_t,  ReservedCodeCacheSize,      32*M);
>>>>
>>>> I would avoid changing these types, otherwise you need to go around and
>>>> clean up a number of other places where it's says it's an uintx,
>>>> like here:
>>>>
>>>> 1909   product_pd(uintx, InitialCodeCacheSize,         \
>>>> 1910           "Initial code cache size (in bytes)")         \
>>>> 1911           range(os::vm_page_size(), max_uintx)         \
>>>>
>>>> Also, it seems you've already added the cast you need for
>>>> InitialCodeCacheSize in codeCache.cpp, so that type change looks
>>>> unnecessary.
>>>>
>>> Done
>>>
>>>
>>>> Btw, patch no longer applies to the latest jdk/jdk.
>>>
>>> Should work now, again.
>>> Tested on s390 31 bit and x86_64  64 bit, testing on s390x underway.
>>>>
>>>>
>>>> cheers,
>>>> Per
>>>>
>>>>>
>>>>> Chris
>>>>>
>>>>
>>>>
>>>>
>>>
>>> Cheers!
>>> Chris
>>>
>>
>>
>>
> Cheers!
> 
> Chris
> 
> 
> 
> 

Ready to  go :

http://cr.openjdk.java.net/~chrisphi/JDK-8203030/webrev.4

Build Details: 2018-06-19-1559053.chrisphi.source
0 Failed Tests
Mach5 Tasks Results Summary

    FAILED: 0
    KILLED: 0
    UNABLE_TO_RUN: 0
    NA: 0
    PASSED: 75
    EXECUTED_WITH_FAILURE: 0

Chris