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

[Langer, Christoph]

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.

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?

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