PATCH: using mixed types in MIN2/MAX2 functions

Wed Jun 18 06:55:37 UTC 2014

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
> >>>>>>>>>>>>>
> >>>>>>>>>