Thanks Dmitry, Kevin for review. Regards, Cheleswer
-----Original Message----- From: Kevin Walls Sent: Friday, April 01, 2016 1:02 AM To: Dmitry Samersoff; Cheleswer Sahu; Mattis Castegren; David Holmes; Daniel Daugherty; hotspot-runtime-...@openjdk.java.net; serviceability-dev@openjdk.java.net Subject: Re: RFR[9u-dev]: 8151442: jstack doesn't close quotation marks properly with threads' name greater than 1996 characters Yes, looks good. 8-) On 31/03/2016 19:54, Dmitry Samersoff wrote: > Cheleswer, > > Looks good for me! (R) > > -Dmitry > > > On 2016-03-31 12:46, Cheleswer Sahu wrote: >> Hi , >> >> I would like to go with the "print_raw()" option as this can print >> any length of thread name. I have modified the code and written a >> test case also for this bug. Please review the code changes from the >> below link >> >> http://cr.openjdk.java.net/~csahu/8151442/webrev.01/ >> >> Regards, >> Cheleswer >> >> -----Original Message----- >> From: Mattis Castegren >> Sent: Wednesday, March 30, 2016 10:42 PM >> To: Kevin Walls; David Holmes; Daniel Daugherty; Dmitry Samersoff; >> Cheleswer Sahu; hotspot-runtime-...@openjdk.java.net; >> serviceability-dev@openjdk.java.net >> Cc: Mattis Castegren >> Subject: RE: RFR[9u-dev]: 8151442: jstack doesn't close quotation >> marks properly with threads' name greater than 1996 characters >> >> Hi >> >> It seems there are two approaches here, either we truncate long thread >> names, or we make sure to print the full thread name. >> >> I agree with Dmitry and Kirk that if the API allows these long names, the >> tooling should do the right thing (even though one has to wonder what these >> long names are). >> >> I suggest we move ahead with the print_raw approach. >> >> If we believe there should be a limit in the Thread name lenghts, I suggest >> we file a new bug for that. >> >> Kind Regards >> /Mattis >> >> -----Original Message----- >> From: Kevin Walls >> Sent: den 24 mars 2016 21:06 >> To: David Holmes; Daniel Daugherty; Dmitry Samersoff; Cheleswer Sahu; >> hotspot-runtime-...@openjdk.java.net; >> serviceability-dev@openjdk.java.net >> Subject: Re: RFR[9u-dev]: 8151442: jstack doesn't close quotation >> marks properly with threads' name greater than 1996 characters >> >> >> Hi >> >> I didn't think of %.XXXXs when Cheleswer and I discussed this briefly. >> I'd like to have suggested that, with the idea that the 2k long thread name >> is extreme, and it's so important that we preserve the format of the output, >> and keep that closing quote, even if we lost some of the thread name. We >> currently and probably always have truncated such names, the problem that >> triggered this was mainly that the format was broken. >> >> As there are several places we pass the name to the stream and could hit the >> length limit, should we have a THREAD_NAME_FORMAT defined for such use >> instead of using %s though the actual length can't be 1996, it's BUFLEN >> minus whatever else we expect to be printed in the same print call. We >> might guess as low as 1024? >> >> (Retaining one st->print() also minimises any risk of concurrent >> prints jumbling up the output.) >> >> Thanks >> Kevin >> >> >> On 21/03/2016 21:24, David Holmes wrote: >>> On 22/03/2016 2:31 AM, Daniel D. Daugherty wrote: >>>> On 3/21/16 2:39 AM, Dmitry Samersoff wrote: >>>>> David, >>>>> >>>>>> I still see %.Ns as the simplest solution - but whatever. >>>>> It spreads artificial limitation of thread name length across >>>>> hotspot sources. >>>>> >>>>> Brief grepping[1] shows couple of other places with the same >>>>> problem and if some days we decide to change default O_BUFLEN, we >>>>> have to not forget to change .N value in couple of places as well. >>>> There should be a way to pass the precision value as a parameter >>>> instead of hardcoding it in the format string. Something like: >>>> >>>> sprintf("%.*s", precision_value, string); >>> Yes the length limit can be passed as a variable. But I think Dmitry >>> just wants to allow for unlimited lengths. Not sure how to achieve >>> that at the lowest level though as we need to avoid complex >>> allocations etc in these print routines. >>> >>> David >>> >>> >>>> Dan >>>> >>>>> 1. >>>>> ./share/vm/prims/jni.cpp >>>>> 673: "in thread \"%s\" ", thread->get_thread_name()); >>>>> >>>>> ./share/vm/runtime/thread.cpp >>>>> 1766: get_thread_profiler()->print(get_thread_name()); >>>>> 1974: get_thread_profiler()->print(get_thread_name()); >>>>> 2896: st->print("\"%s\" ", get_thread_name()); >>>>> 2926: st->print("%s", get_thread_name_string(buf, buflen)); >>>>> 2932: st->print("JavaThread \"%s\"", get_thread_name_string(buf, >>>>> buflen)); >>>>> >>>>> >>>>> ./share/vm/services/threadService.cpp >>>>> 882: ... st->print_cr("\"%s\":", >>>>> currentThread->get_thread_name()); >>>>> 919: ..."%s \"%s\"", owner_desc, >>>>> currentThread->get_thread_name()); >>>>> 932: ... st->print_cr("\"%s\":", >>>>> currentThread->get_thread_name()); >>>>> >>>>> -Dmitry >>>>> >>>>> >>>>> On 2016-03-19 00:37, David Holmes wrote: >>>>>> On 18/03/2016 11:28 PM, Dmitry Samersoff wrote: >>>>>>> David, >>>>>>> >>>>>>>> Ignoring Dmitry's issue it still seems simpler/cleaner to just >>>>>>>> add the desired precision value to the %s than to split into >>>>>>>> two print statements. Or bite the bullet now and make the >>>>>>>> length immaterial by breaking the name into chunks. It's as >>>>>>>> easy to fix as to write the RFE :) >>>>>>> For this particular case the simplest solution is to use print_raw: >>>>>>> >>>>>>> print_raw("\""\"); print_raw(get_thread_name()); >>>>>>> print_raw("\""\"); >>>>>> I still see %.Ns as the simplest solution - but whatever. >>>>>> >>>>>>> But as soon as print() finally ends up with: >>>>>>> >>>>>>> const int written = vsnprintf(buffer, buflen, format, ap); ... >>>>>>> DEBUG_ONLY(warning("increase O_BUFLEN in ostream.hpp -- output >>>>>>> truncated");) >>>>>>> >>>>>>> Complete fix should be something like: >>>>>>> >>>>>>> int desired_size = vsnprintf(NULL, 0, format, ap); if >>>>>>> (desired_size > O_BUFLEN) { >>>>>>> malloc >>>>>>> .... >>>>>>> } >>>>>>> >>>>>>> but it has performance penalty, so we should use some tricks to >>>>>>> cover most common %s/%d/%p cases. >>>>>> So you want to remove the internal limitation instead of have the >>>>>> clients deal with it? Not sure it is worth the effort - IIRC that >>>>>> code can be used at sensitive times hence the simple approach to >>>>>> buffer management. >>>>>> >>>>>> David >>>>>> >>>>>>> -Dmitry >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 2016-03-18 15:51, David Holmes wrote: >>>>>>>> On 18/03/2016 10:03 PM, Cheleswer Sahu wrote: >>>>>>>>> Hi David, >>>>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: David Holmes >>>>>>>>> Sent: Friday, March 18, 2016 2:42 PM >>>>>>>>> To: Cheleswer Sahu; hotspot-runtime-...@openjdk.java.net; >>>>>>>>> serviceability-dev@openjdk.java.net >>>>>>>>> Subject: Re: RFR[9u-dev]: 8151442: jstack doesn't close >>>>>>>>> quotation marks properly with threads' name greater than 1996 >>>>>>>>> characters >>>>>>>>> >>>>>>>>> On 18/03/2016 5:54 PM, Cheleswer Sahu wrote: >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> Please review the code changes for >>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8151442. >>>>>>>>>> >>>>>>>>>> Webrev Link: http://cr.openjdk.java.net/~csahu/8151442/ >>>>>>>>>> >>>>>>>>>> Bug Brief: >>>>>>>>>> >>>>>>>>>> In jstack thread dumps , thread name greater than 1996 >>>>>>>>>> characters doesn't close quotation marks properly. >>>>>>>>> Anyone giving a thread a name that long deserves to get a core >>>>>>>>> dump! >>>>>>>>> ;-) >>>>>>>>> >>>>>>>>>> Problem Identified: >>>>>>>>>> >>>>>>>>>> Jstack is using below code to print thread name >>>>>>>>>> >>>>>>>>>> src/share/vm/runtime/thread.cpp >>>>>>>>>> >>>>>>>>>> void JavaThread::print_on(outputStream *st) const { >>>>>>>>>> >>>>>>>>>> st->print("\"%s\" ", get_thread_name()); >>>>>>>>>> >>>>>>>>>> Here "st->print()" internally uses max buffer length as >>>>>>>>>> O_BUFLEN (2000). >>>>>>>>>> >>>>>>>>>> void >>>>>>>>>> outputStream::do_vsnprintf_and_write_with_automatic_buffer(co >>>>>>>>>> ns >>>>>>>>>> t >>>>>>>>>> char* format, va_list ap, bool add_cr) { >>>>>>>>>> >>>>>>>>>> char buffer[O_BUFLEN]; >>>>>>>>>> >>>>>>>>>> do_vsnprintf_and_write_with_automatic_buffer() finally calls >>>>>>>>>> "vsnprintf()" which truncates the anything greater >>>>>>>>>> than the max size(2000). In this case thread's name(> 1996) >>>>>>>>>> along with quotation marks (2) >>>>>>>>>> >>>>>>>>>> plus one terminating character exceeds the max buffer size >>>>>>>>>> (2000), therefore the closing quotation marks gets truncated. >>>>>>>>>> >>>>>>>>>> Solution: >>>>>>>>>> >>>>>>>>>> Split the "st->print("\"%s\" ", get_thread_name())" in two >>>>>>>>>> statements >>>>>>>>>> >>>>>>>>>> 1.st->print("\"%s", get_thread_name()); >>>>>>>>>> >>>>>>>>>> 2.st->print("\" "); >>>>>>>>>> >>>>>>>>>> This will ensure presence of closing quotation mark always. >>>>>>>>> Can't we just limit the number of characters read by %s? >>>>>>>>> >>>>>>>>> Yes we can do it, but just one thing which I think of is, if >>>>>>>>> the truncation of output inside output stream issue get >>>>>>>>> resolves as Dmritry suggested or O_BUFFLEN size gets increased >>>>>>>>> in any future fix then we have to again make changes in this >>>>>>>>> code, as limiting the no. >>>>>>>>> of character read by %s will give truncated output . In such >>>>>>>>> case present fix will have no effect. >>>>>>>> Ignoring Dmitry's issue it still seems simpler/cleaner to just >>>>>>>> add the desired precision value to the %s than to split into >>>>>>>> two print statements. Or bite the bullet now and make the >>>>>>>> length immaterial by breaking the name into chunks. It's as >>>>>>>> easy to fix as to write the RFE :) >>>>>>>> >>>>>>>> David >>>>>>>> >>>>>>>>> David >>>>>>>>> >>>>>>>>>> Regards, >>>>>>>>>> >>>>>>>>>> Cheleswer >>>>>>>>>> >
