8364103: Convert existing sprintf-chains to stringStream #26894
Conversation
|
👋 Welcome back Arraying! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
changed the title
There was a problem hiding this comment.
Why are we printing to a buffer first?
Originally this function was java_lang_Throwable::print_stack_element_to_buffer() and returns a buffer. Hence the anachronistic comment "// Print stack trace element to resource allocated buffer" which is no longer correct.
I can't find any information from the history of this file:
I am guessing the reason is to avoid interleaving with other threads. I think the above comment should be updated to reflect this.
| const size_t buf_size = buf_len + 64; | ||
| char* buf = NEW_RESOURCE_ARRAY(char, buf_size); | ||
| // The string stream that will handle all of the formatting and outputting. | ||
| stringStream ss; |
| @@ -2588,75 +2588,70 @@ static void print_stack_element_to_stream(outputStream* st, Handle mirror, int m | |||
| int version, int bci, Symbol* name) { | |||
| ResourceMark rm; | |||
|
|
|||
| // Get strings and string lengths | |||
| // Get strings. | |||
There was a problem hiding this comment.
This comment is useless and should be removed.
|
|
||
| // Print stack trace line in buffer | ||
| size_t buf_off = os::snprintf_checked(buf, buf_size, "\tat %s.%s(", klass_name, method_name); | ||
| ss.print("\tat %s.%s(", klass_name, method_name); |
There was a problem hiding this comment.
The comment about this line is useless and should be removed.
|
|
||
| // Print module information | ||
| if (module_name != nullptr) { | ||
| if (module_version != nullptr) { | ||
| buf_off += os::snprintf_checked(buf + buf_off, buf_size - buf_off, "%s@%s/", module_name, module_version); | ||
| ss.print("%s@%s/", module_name, module_version); |
There was a problem hiding this comment.
Here and other places, this change is going to merge-conflict with #26849.
I think that PR should be allowed to go first.
Hi all,
This PR refactors
javaClasses'print_stack_element_to_streamto use astringStreaminstead of manually maintaining a buffer withos::snprintfchains.The JBS issue outlines to do this for all occurrences of
snprintf-like chains. The majority of the calls toos::snprintf,os::snprintf_checked,os::vsnprintf, etc. occur just once.LogTagSet::vwriteis the only place to my knowledge where there remains avsnprintfchain using manual buffer arithmetic. After consulting @jdksjolen, we decided that due to thea) low-level nature of the code, and
b) widespread usage of logging in the VM,
this should probably warrant a larger discussion and/or separate issue. Personally, I feel like the performance & correctness risks of introducing a high-level abstraction here outweighs the benefits.
I've run tests with JDK tiers 1-3 on macOS (AArch64, x64), Linux (AArch64, x64), and Windows (x64); all green.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26894/head:pull/26894$ git checkout pull/26894Update a local copy of the PR:
$ git checkout pull/26894$ git pull https://git.openjdk.org/jdk.git pull/26894/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26894View PR using the GUI difftool:
$ git pr show -t 26894Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26894.diff
Using Webrev
Link to Webrev Comment