Rachel, On 2015-10-28 17:48, Rachel Protacio wrote: > Thank you for the comments, both Dmitry and Mikael. Replies inline. > > Modified webrev: http://cr.openjdk.java.net/~rprotacio/8138916.01/
111 Assert is never happens on Windows, so it's better to move it to os_posix 113 newbuf_len doesn't account prefix_len 115 you may consider to use pre-calculated at ll. 108 prefix rather than calculate it again. 116 int ret redefined it causes a warning. -Dmitry > (Retested and works fine.) > > On 10/27/2015 7:52 AM, Dmitry Samersoff wrote: >> Rachel, >> >> In worst case (windows, overflow) the code would parse format string >> four times. > You're right. I've added a check. New amount of times vnsprintf() is > called: > posix, fits => 1 > posix, overflow => 2 > windows, fits => 1 > windows, overflow => 3 (the best performance we can get) > >> Also NEW_C_HEAP_ARRAY takes a lock and might become a >> performance bottleneck. >> >> POSIX equivalent of _vscprintf(fmt, args) is vsnprintf(NULL, 0, fmt, >> args) >> >> So it might be better to create os::log_vcprintf(fmt, args), then write >> code at log.hpp as: >> >> int buf_len = os::log_vcprintf(fmt, args); >> assert(buf_len < 2*page_size, "Stack overflow in logging"); >> char buf[buf_len]; >> vsprintf(buf, fmt, args); > I don't think this would work anyway. A char[] needs a static size. > Thanks, > Rachel >> -Dmitry >> >> >> >> On 2015-10-26 23:58, Rachel Protacio wrote: >>> Hello, all, >>> >>> Please review this fix to the faulty write function from the Unified >>> Logging framework. >>> >>> Summary: In logging/log.hpp, the logging vwrite function previously >>> asserted that the buffer remains within 512 characters, which is too >>> short for logging message of non-pre-determined length, e.g. for vtable >>> and itable function names. Now, the function resizes the buffer to the >>> appropriate length, asserting only that the resulting buffer is valid. >>> Includes tag "logtest" to test that logging can print an output of 518 >>> characters. >>> >>> Open webrev:http://cr.openjdk.java.net/~rprotacio/8138916/ >>> Bug:https://bugs.openjdk.java.net/browse/JDK-8138916 >>> >>> Includes own acceptance test, and passes JPRT and remote hotspot/test >>> tests. >>> >>> Thank you, >>> Rachel > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
