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

David Holmes david.holmes at oracle.com
Fri Apr 20 08:00:31 UTC 2018


On 20/04/2018 5:33 PM, Langer, Christoph wrote:
> Hi David,
> 
> thanks a lot for taking this deep dive - somebody outside Oracle would not have access to this information. So I am glad that we understand the backgrounds better now.

I was just hoping it would be someone else inside Oracle :)

> 
> The patch is running through jdk/submit currently, will push if everything runs fine. Our internal testing did not show regressions.
> 
> One question, as I'm planning to finish my work on this with another item to clean up the jio_*print* functions: You think that the raw write could be replaced with a call to vfprintf under this circumstances? Shouldn't the risk of interspersing characters be higher with vfprintf compared to raw write?

I would expect vfprintf to expand everything before itself doing a write 
to the underlying stream ... but I haven't looked at the details of the 
stream implementation. The interspersing, actually I should have said 
interleaving, was an issue of using putchar rather than write.

I don't think it will make a difference if we only use the vprintf.

David

> Best regards
> Christoph
> 
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Freitag, 20. April 2018 03:39
>> To: Langer, Christoph <christoph.langer at sap.com>; hotspot-
>> dev at openjdk.java.net
>> Cc: daniel.daugherty at oracle.com; hotspot-runtime-dev at openjdk.java.net;
>> Thomas Stüfe <thomas.stuefe at gmail.com>
>> Subject: Re: RFR(S): 8201649: Remove dubious call_jio_print in ostream.cpp
>>
>> 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
>>>>>


More information about the hotspot-runtime-dev mailing list