RFR(S): 8201649: Remove dubious call_jio_print in ostream.cpp

[David Holmes]

Hi Christoph,

I managed to find some time to dig deeper ...

On 19/04/2018 11:55 PM, Langer, Christoph wrote:
> Hi,
> 
> with my means of searching in the public OpenJDK repositories I can only find that 'call_jio_print' has always been there. No idea about the backgrounds of why we need this buffer copy approach instead of using jio_printf directly.

Some relatively recent history:

https://bugs.openjdk.java.net/browse/JDK-6518269

The real history:

- April 2000: jio_print was introduced to "Make print more reliable 
inside signal handler." It was actually a conversion from a character 
version:

// HotSpot specific jio method
void jio_putchar(int c) {
   if (Arguments::vfprintf_hook() != NULL) {
     jio_fprintf(stdout, "%c", c);
   } else {
     putchar(c);
   }
}

to the current string version. That's what the 'atomic' reference is 
about - printing the entire string instead of char-by-char which could 
lead to interspersing.

- The putchar version was added in Jun 1999.

 From what I can see the putchar version either went the jio_fprintf 
path if the hook was installed else a raw pucthar. The string version 
then followed suit.

With that in mind I don't think the "raw write" has any significance.

So this is Reviewed.

Thanks,
David
-----

> 
> So, can someone else please help reviewing http://cr.openjdk.java.net/~clanger/webrevs/8201649.0/ ?
> 
> Thanks
> Christoph
> 
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Donnerstag, 19. April 2018 14:07
>> To: Langer, Christoph <christoph.langer at sap.com>; Thomas Stüfe
>> <thomas.stuefe at gmail.com>
>> Cc: daniel.daugherty at oracle.com; hotspot-dev at openjdk.java.net; hotspot-
>> runtime-dev at openjdk.java.net
>> Subject: Re: RFR(S): 8201649: Remove dubious call_jio_print in ostream.cpp
>>
>> Hi Christoph,
>>
>> On 19/04/2018 9:08 PM, Langer, Christoph wrote:
>>> Hi Thomas, David,
>>>
>>>>> @David: No, I don't think it needs to be dialed back at this point, but I
>>>> agree, the bug should be updated somewhat to the current scope by
>>>> mentioning that the goal is to get rid of jio_print and jio_printf completely
>>>> (along with call_jio_print in ostream.cpp). Those 2 functions were just 2
>>>> hooks used in ostream.cpp and got routed through jio_vfprintf anyway,
>>>> except for the raw write case when no vfprint_hook was installed - and
>> we'll
>>>> preserve that.
>>
>> You preserved it in the sense it's still there, but you changed two call
>> sites so they can never now hit it. So I'm back to the unanswerable
>> "does that matter?".
>>
>> Unless someone can do the archaeology on this I'm going to have to abstain.
>>
>> David
>> -----
>>
>>>>>
>>>>
>>>> Hm. Instead of broading the scope of the issue, I would actually
>>>> prefer the bug description to be clarified and limited to the original
>>>> problem we tried to solve. Because that was a real valid problem.
>>>>
>>>> If only to get David's full review and not to confuse him further :)
>>>
>>> Ok, I agree. So I tried to clarify the bug a bit more in its original sense. And
>> we'd be back to webrev #0.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8201649
>>> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8201649.0/
>>>
>>> The change of the webrev is missing a cast in line 2729 - (len needs to be
>> casted int). This produces a build warning which turns into an error in some
>> platforms. I'll have that fixed in the submitted version.
>>>
>>> For the further cleanup of jio_print methods I'll probably file another bug.
>>>
>>> As I think we already had a principal agreement about this proposal earlier,
>> I would consider this reviewed by stuefe and dholmes if I don't hear back
>> from you today. I would push this tomorrow after push to jdk-submit and
>> further internal testing here at SAP.
>>>
>>> Thanks
>>> Christoph
>>>