PATCH: using mixed types in MIN2/MAX2 functions

Wed Jun 18 12:47:12 UTC 2014

Hi Dan,

I have uploaded your webrev here:

http://cr.openjdk.java.net/~jwilhelm/8046938/webrev/

The change looks good to me.
/Jesper

Dan Horák skrev 18/6/14 08:55:
> On Mon, 16 Jun 2014 19:01:20 +0200
> Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>
>> Hi Dan,
>>
>> I created a bug four this issue:
>> https://bugs.openjdk.java.net/browse/JDK-8046938
>> /Jesper
>
> updated webrev is now at http://fedora.danny.cz/openjdk/size_t-2/webrev/
> (and http://fedora.danny.cz/openjdk/size_t-2/webrev.zip)
>
> changes:
> - refreshed for current hs-gc repository
> - dropped unnecessary typecast in get_hum_bytes() (concurrentMark.cpp)
> - typecasts in FLAG_SET_ERGO() changed to uintx from size_t
> 	(arguments.cpp)
>
>
> 	With regards
>
> 		Dan
>
>> Dan Horák skrev 16/6/14 18:29:
>>> On Mon, 16 Jun 2014 17:26:24 +0200
>>> Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>>
>>>> Hi Dan,
>>>>
>>>> Since you got a comment from Bengt maybe you should fix that issue
>>>> before I upload the new webrev.
>>>>
>>>> I looked through the webrev and I have one additional comment. I
>>>> would prefer if the changes in arguments.cpp was the other way,
>>>> casting stuff to uintx instead of size_t, since we use the result
>>>> of MAX and MIN to set flags of type uintx.
>>>
>>> I wasn't sure what would be better approach here
>>>
>>>> When I applied the webrev a change in
>>>> src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp got in the
>>>> way of the patch. The patch still applies but with an offset, so
>>>> patch complains. If you build a new webrev with the changes above,
>>>> also pull down the latest hs-gc to get the g1CollectedHeap.cpp
>>>> change.
>>>
>>> thanks for the comments, I'll prepare new webrev hopefully tomorrow
>>>
>>>
>>> 		Dan
>>>
>>>> Thanks!
>>>> /Jesper
>>>>
>>>>
>>>> Dan Horák skrev 16/6/14 16:21:
>>>>> On Mon, 16 Jun 2014 16:19:17 +0200
>>>>> Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>>>>
>>>>>> Dan,
>>>>>>
>>>>>> I can host the webrev. Send me the zipped webrev archive and I'll
>>>>>> upload it to cr.openjdk.
>>>>>
>>>>> Jesper, thanks a lot, the whole archive is at
>>>>> http://fedora.danny.cz/openjdk/size_t-1/webrev.zip
>>>>>
>>>>>
>>>>> 		Dan
>>>>>
>>>>>> /Jesper
>>>>>>
>>>>>> David Holmes skrev 16/6/14 13:15:
>>>>>>> On 16/06/2014 5:17 PM, Dan Horák wrote:
>>>>>>>> On Mon, 16 Jun 2014 16:31:29 +1000
>>>>>>>> David Holmes <david.holmes at oracle.com> wrote:
>>>>>>>>
>>>>>>>>> On 13/06/2014 10:55 PM, Dan Horák wrote:
>>>>>>>>>> On Fri, 13 Jun 2014 14:10:53 +0200
>>>>>>>>>> Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
>>>>>>>>>>> Then a formality comment. I think you have to post the
>>>>>>>>>>> webrev on cr.openjdk.java.net to get the legal stuff
>>>>>>>>>>> correct.
>>>>>>>>>>
>>>>>>>>>> hm, I've used this procedure (my webrev, then someone
>>>>>>>>>> privileged takes it and pushes it) few times in the past and
>>>>>>>>>> it worked.
>>>>>>>>>
>>>>>>>>> We've been advised that all contributions have to come in via
>>>>>>>>> OpenJDK infrastructure - either a webrev on
>>>>>>>>> cr.openjdk.java.net or as attachments to mailing list mails.
>>>>>>>>
>>>>>>>> I understand, but AFAIK cr.openjdk.java.net is accessible only
>>>>>>>> to people with push access, which I'm not.
>>>>>>>
>>>>>>> No you only need Author status, not Committer.
>>>>>>>
>>>>>>>> So the question is will the list
>>>>>>>> accept 3MB attachment? It is the size of the zipped webrev. Or
>>>>>>>> will a plain text patch suffice?
>>>>>>>
>>>>>>> For large webrevs you may be able to find someone to host it for
>>>>>>> you on cr.openjdk. I don't know what limit exists on the mailing
>>>>>>> list but I'd hate to see very large patches submitted that way.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> David
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>            Dan
>>>>>>>>
>>>>>>>>> David
>>>>>>>>> -----
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>            Dan
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Bengt
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2014-06-13 09:55, Dan Horák wrote:
>>>>>>>>>>>> On Thu, 12 Jun 2014 21:49:40 +0200
>>>>>>>>>>>> Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Which repository is the patch built on? Since it is mostly
>>>>>>>>>>>>> GC code it would be good if it goes in through jdk9/hs-gc.
>>>>>>>>>>>>>
>>>>>>>>>>>>> The changes looks good in the webrev, but the patch did
>>>>>>>>>>>>> not apply to a recent jdk9/hs-gc. Especially the collector
>>>>>>>>>>>>> policy code has changed a lot recently.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I would prefer to look at the changes applied in my
>>>>>>>>>>>>> workspace before approving it, so if you could update to a
>>>>>>>>>>>>> recent hs-gc it would be great.
>>>>>>>>>>>> I have refreshed the patch for the hs-gc repository, please
>>>>>>>>>>>> see http://fedora.danny.cz/openjdk/size_t-1/webrev/
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>        Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>>            Dan
>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>> /Jesper
>>>>>>>>>>>>>
>>>>>>>>>>>>> Vladimir Kozlov skrev 12/6/14 20:57:
>>>>>>>>>>>>>> Thank you, Dan
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I think casting is acceptable solution. Someone in GC
>>>>>>>>>>>>>> group should review and sponsor this since you touched
>>>>>>>>>>>>>> GC code mostly.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>> Vladimir
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 6/12/14 2:05 AM, Dan Horák wrote:
>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> this the last and largest part of my changes required on
>>>>>>>>>>>>>>> s390 (32-bit) to build OpenJDK out of the box. The
>>>>>>>>>>>>>>> MIN2/MAX2 functions are implemented using templates and
>>>>>>>>>>>>>>> require both arguments to be of the same type. 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).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> As a solution [1] I chose to typecast the smaller type
>>>>>>>>>>>>>>> (uintx) to size_t which is of same or larger size.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> [1] http://fedora.danny.cz/openjdk/size_t/webrev/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Note: I'm Red Hat employee (dhorak at redhat.com), so I
>>>>>>>>>>>>>>> should be covered by Red Hat's CLA for my contributions.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>           With regards
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>               Dan
>>>>>>>>>>>>>>>
>>>>>>>>>>>