[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8198844 Clean up GensrcX11Wrappers
From: Phil Race <philip.race () oracle ! com>
Date: 2018-03-06 0:04:21
Message-ID: e8600b4b-c330-77d2-fd1f-6bc4e989fee1 () oracle ! com
[Download RAW message or body]
It isn't "unlikely" .. it is just "relatively rare".
-phil.
On 03/05/2018 04:00 PM, Magnus Ihse Bursie wrote:
>
>
> On 2018-03-06 00:53, Phil Race wrote:
>> + $(ECHO) needs to be updated for both 32 and 64 bit platforms. You
>> have now needs -> need.
> Fixed typo. Thanks! (I really hate how I suck at those English plural
> s'es :-&)
>> Could we make this easier by trying to generate the 32 bit native
>> binary as well if running on 64 bits ? Looks like this was not
>> normally done on Linux but if you have 32 bit compiler + library
>> support it should work. Or do you consider it easy enough to just
>> kick off a 32 bit build to get the same ?
> Yes, I consider that easy enough, given how unlikely it is that this
> needs ever be done.
>
> /Magnus
>
>> -phil.
>>
>> On 03/05/2018 03:30 PM, Magnus Ihse Bursie wrote:
>>>
>>> On 2018-03-05 23:38, Phil Race wrote:
>>>> >I'm not sure what role the "verification" step we had before ever
>>>> played.
>>>> >For all the years we've been "verifying" this, we've detected no
>>>> differences.
>>>>
>>>> I think this is useful in the event that you make some changes and
>>>> regenerate the 64 bit sizes but not 32 bit.
>>> That's a good point. I should add a warning message when
>>> regenerating the checked-in data files that you need to regenerate
>>> the files on both 32 and 64 bit platforms.
>>>>
>>>> For example this old bug similarly reported a breakage on Solaris ..
>>>> https://bugs.openjdk.java.net/browse/JDK-6804680
>>>>
>>>> ... back in the day when we only had that checked in and the ones
>>>> used for Linux
>>>> were being generated on the fly. My reading of the history here is
>>>> sizes.32 and sizes.64
>>>> were added to support cross-compilation, and the verification step
>>>> meant that all
>>>> architectures would get checked in some build or other.
>>>
>>> I'd rather say that it was at this time that we stopped run-time
>>> generation of the X11 size data. The "verification" step was there
>>> more like a comfort thing for the build team, since we was too
>>> conservative.
>>>
>>>
>>>> The clean up of removing the solaris specific seems like it could
>>>> have been
>>>> done a long time ago .. I am not sure why was ever only this one
>>>> case there.
>>>> I'd have to dig back a very long way.
>>>>
>>>> I do agree we do not need to support 32 + 64 bit concurrently, that
>>>> went
>>>> away when 64 bit solaris overlay on 32 bit was dropped.
>>>
>>> Thank you.
>>>
>>> Updated webrev with warning message about updating for all platforms:
>>>
>>> http://cr.openjdk.java.net/~ihse/JDK-8198844-clean-up-GensrcX11Wrappers/webrev.03
>>>
>>>
>>> (Only UpdateX11Wrappers.gmk has changed)
>>>
>>> /Magnus
>>>
>>>>
>>>> -phil.
>>>>
>>>> On 03/02/2018 07:23 AM, Erik Joelsson wrote:
>>>>> Adding 2d-dev in the hopes of getting some input from component
>>>>> team. Seems like they should be aware of us removing the support
>>>>> for multiple data models.
>>>>>
>>>>> Looks like you left a debug message at line 40 in
>>>>> GensrcX11Wrappers.gmk.
>>>>>
>>>>> /Erik
>>>>>
>>>>> On 2018-03-02 03:00, Magnus Ihse Bursie wrote:
>>>>>> On 2018-03-02 00:02, Erik Joelsson wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> In xlibtypes.txt comment, should it be sizes-64.txt?
>>>>>>
>>>>>> Yes, good catch.
>>>>>>
>>>>>>>
>>>>>>> Generating both 32 and 64 seems a bit outdated at this point.
>>>>>>> Surely this is a remnant of bundling 64 and 32 bit together on
>>>>>>> Solaris in the past? Perhaps someone in 2d can answer this?
>>>>>>> Would be nice to be able to clean up that part as well if possible.
>>>>>> Yes, you are right. We should clean up that as well. I was just
>>>>>> too conservative. I've now actually checked what happens when you
>>>>>> generate both 32 and 64 bit versions, and the result is that
>>>>>> instead of code like:
>>>>>> public static int getSize() { return 96; }
>>>>>> we get code like this:
>>>>>> public static int getSize() { return ((XlibWrapper.dataModel
>>>>>> == 32)?(80):(96)); }
>>>>>>
>>>>>> Since we do no longer support multiple data models for the same
>>>>>> build, this is just unnecessary. In fact, that leads to an even
>>>>>> better cleanup, since we will always need just a single input file.
>>>>>>
>>>>>> I also wrapped the tool calls in ExecuteWithLog.
>>>>>>
>>>>>> Updated webrev:
>>>>>> http://cr.openjdk.java.net/~ihse/JDK-8198844-clean-up-GensrcX11Wrappers/webrev.02
>>>>>>
>>>>>>
>>>>>> /Magnus
>>>>>>
>>>>>
>>>>
>>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
It isn't "unlikely" .. it is just "relatively rare".<br>
<br>
-phil.<br>
<br>
<div class="moz-cite-prefix">On 03/05/2018 04:00 PM, Magnus Ihse
Bursie wrote:<br>
</div>
<blockquote type="cite"
cite="mid:7e270b69-9b1f-81e0-bf28-820e893b66ad@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<br>
<br>
<div class="moz-cite-prefix">On 2018-03-06 00:53, Phil Race wrote:<br>
</div>
<blockquote type="cite"
cite="mid:5b0e7911-b27d-ce35-abde-a35faaf89693@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
<pre><span class="new">+ $(ECHO) needs to be updated for both 32 and \
64 bit platforms. You have now
needs -> need.</span></pre>
</blockquote>
Fixed typo. Thanks! (I really hate how I suck at those English
plural s'es :-&)<br>
<blockquote type="cite"
cite="mid:5b0e7911-b27d-ce35-abde-a35faaf89693@oracle.com">
<pre><span class="new">
Could we make this easier by trying to generate the 32 bit native binary as well if \
running on 64 bits ? Looks like this was not normally done on Linux but if you have \
32 bit compiler + library support it should work.
Or do you consider it easy enough to just kick off a 32 bit build to get the same \
?</span></pre> </blockquote>
Yes, I consider that easy enough, given how unlikely it is that
this needs ever be done.<br>
<br>
/Magnus<br>
<br>
<blockquote type="cite"
cite="mid:5b0e7911-b27d-ce35-abde-a35faaf89693@oracle.com">
<pre><span class="new">
-phil.
</span></pre>
<br>
<div class="moz-cite-prefix">On 03/05/2018 03:30 PM, Magnus Ihse
Bursie wrote:<br>
</div>
<blockquote type="cite"
cite="mid:c1af2076-970f-7058-48c3-72bb0a261c14@oracle.com"> <br>
On 2018-03-05 23:38, Phil Race wrote: <br>
<blockquote type="cite">>I'm not sure what role the
"verification" step we had before ever played. <br>
>For all the years we've been "verifying" this, we've
detected no differences. <br>
<br>
I think this is useful in the event that you make some
changes and <br>
regenerate the 64 bit sizes but not 32 bit. <br>
</blockquote>
That's a good point. I should add a warning message when
regenerating the checked-in data files that you need to
regenerate the files on both 32 and 64 bit platforms. <br>
<blockquote type="cite"> <br>
For example this old bug similarly reported a breakage on
Solaris .. <br>
<a class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-6804680"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-6804680</a>
<br>
<br>
... back in the day when we only had that checked in and the
ones used for Linux <br>
were being generated on the fly. My reading of the history
here is sizes.32 and sizes.64 <br>
were added to support cross-compilation, and the
verification step meant that all <br>
architectures would get checked in some build or other. <br>
</blockquote>
<br>
I'd rather say that it was at this time that we stopped
run-time generation of the X11 size data. The "verification"
step was there more like a comfort thing for the build team,
since we was too conservative. <br>
<br>
<br>
<blockquote type="cite">The clean up of removing the solaris
specific seems like it could have been <br>
done a long time ago .. I am not sure why was ever only this
one case there. <br>
I'd have to dig back a very long way. <br>
<br>
I do agree we do not need to support 32 + 64 bit
concurrently, that went <br>
away when 64 bit solaris overlay on 32 bit was dropped. <br>
</blockquote>
<br>
Thank you. <br>
<br>
Updated webrev with warning message about updating for all
platforms: <br>
<br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eihse/JDK-8198844-clean-up-GensrcX11Wrappers/webrev.03"
moz-do-not-send="true">http://cr.openjdk.java.net/~ihse/JDK-8198844-clean-up-GensrcX11Wrappers/webrev.03</a>
<br>
<br>
(Only UpdateX11Wrappers.gmk has changed) <br>
<br>
/Magnus <br>
<br>
<blockquote type="cite"> <br>
-phil. <br>
<br>
On 03/02/2018 07:23 AM, Erik Joelsson wrote: <br>
<blockquote type="cite">Adding 2d-dev in the hopes of
getting some input from component team. Seems like they
should be aware of us removing the support for multiple
data models. <br>
<br>
Looks like you left a debug message at line 40 in
GensrcX11Wrappers.gmk. <br>
<br>
/Erik <br>
<br>
On 2018-03-02 03:00, Magnus Ihse Bursie wrote: <br>
<blockquote type="cite">On 2018-03-02 00:02, Erik Joelsson
wrote: <br>
<blockquote type="cite">Hello, <br>
<br>
In xlibtypes.txt comment, should it be sizes-64.txt? <br>
</blockquote>
<br>
Yes, good catch. <br>
<br>
<blockquote type="cite"> <br>
Generating both 32 and 64 seems a bit outdated at this
point. Surely this is a remnant of bundling 64 and 32
bit together on Solaris in the past? Perhaps someone
in 2d can answer this? Would be nice to be able to
clean up that part as well if possible. <br>
</blockquote>
Yes, you are right. We should clean up that as well. I
was just too conservative. I've now actually checked
what happens when you generate both 32 and 64 bit
versions, and the result is that instead of code like: <br>
public static int getSize() { return 96; } <br>
we get code like this: <br>
public static int getSize() { return
((XlibWrapper.dataModel == 32)?(80):(96)); } <br>
<br>
Since we do no longer support multiple data models for
the same build, this is just unnecessary. In fact, that
leads to an even better cleanup, since we will always
need just a single input file. <br>
<br>
I also wrapped the tool calls in ExecuteWithLog. <br>
<br>
Updated webrev: <br>
<a class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eihse/JDK-8198844-clean-up-GensrcX11Wrappers/webrev.02"
moz-do-not-send="true">http://cr.openjdk.java.net/~ihse/JDK-8198844-clean-up-GensrcX11Wrappers/webrev.02</a>
<br>
<br>
/Magnus <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic