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

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 jioprint 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 calljioprint 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 'calljioprint' has always been there. No idea about the backgrounds of why we need this buffer copy approach instead of using jioprintf directly. Some relatively recent history: https://bugs.openjdk.java.net/browse/JDK-6518269 The real history: - April 2000: jioprint was introduced to "Make print more reliable inside signal handler." It was actually a conversion from a character version: // HotSpot specific jio method void jioputchar(int c) { if (Arguments::vfprintfhook() != NULL) { jiofprintf(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 jiofprintf 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 calljioprint 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 jioprint and jioprintf completely (along with calljioprint in ostream.cpp). Those 2 functions were just 2 hooks used in ostream.cpp and got routed through jiovfprintf anyway, except for the raw write case when no vfprinthook 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 jioprint 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-dev mailing list