8310170: Use sp's argument to improve performance of outputStream::indent and remove SP_USE_TABS #14502

jdksjolen · 2023-06-15T20:50:11Z

Hi,

Please consider this small enhancement. sp takes a count argument, this was never used by indent, let's just use it.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8310170: Use sp's argument to improve performance of outputStream::indent and remove SP_USE_TABS (Enhancement - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to d9ee4c2f
David Holmes (@dholmes-ora - Reviewer)
Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14502/head:pull/14502
$ git checkout pull/14502

Update a local copy of the PR:
$ git checkout pull/14502
$ git pull https://git.openjdk.org/jdk.git pull/14502/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14502

View PR using the GUI difftool:
$ git pr show -t 14502

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14502.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-06-15T20:51:12Z

👋 Welcome back jsjolen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-06-15T20:53:06Z

@jdksjolen The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

jdksjolen · 2023-06-16T08:36:43Z

/label add hotspot-runtime

openjdk · 2023-06-16T08:38:18Z

@jdksjolen
The hotspot-runtime label was successfully added.

mlbridge · 2023-06-16T08:42:21Z

Webrevs

01: Full - Incremental (329e8324)
00: Full (d9ee4c2f)

shipilev

Looks okay. I thought what would happen if _indentation < _position, but sp() seems to handle count < 0 fine.

openjdk · 2023-06-16T13:06:56Z

@jdksjolen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8310170: Use sp's argument to improve performance of outputStream::indent and remove SP_USE_TABS

Reviewed-by: shade, dholmes, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 250 new commits pushed to the master branch:

7b3c2dc: 8311122: Fix typos in java.base
607ddaa: 8310997: missing @SInCE tags in jdk.httpserver
7655b48: 8311001: missing @SInCE info in jdk.net
711cddd: 8311249: Remove unused MemAllocator::obj_memory_range
514816e: 8309889: [s390] Missing return statement after calling jump_to_native_invoker method in generate_method_handle_dispatch.
60544f9: 8309894: compiler/vectorapi/VectorLogicalOpIdentityTest.java fails on SVE system with UseSVE=0
0916e6a: 8311092: Please disable runtime/jni/nativeStack/TestNativeStack.java on armhf
d8a0121: 8311109: tautological-compare warning in awt_Win32GraphicsDevice.cpp
b9198f9: 8254566: Clarify the spec of ClassLoader::getClassLoadingLock for non-parallel capable loader
f393975: 8310743: assert(reserved_rgn != nullptr) failed: Add committed region, No reserved region found
... and 240 more: https://git.openjdk.org/jdk/compare/955fc2fabafc4c4d79d137bf86194228d5cf8ad8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

dholmes-ora

There's potentially a subtle difference in behaviour here if SP_USE_TABS were ever true - but as it is defined to be false there is no difference in practice. Maybe we should just cull the code related to SP_USE_TABS and not pretend we could do something different here?

Thanks.

jdksjolen · 2023-07-04T12:17:47Z

Hi,

Sorry for the slow response from me on this.

@dholmes-ora, let's remove SP_USE_TABS in this PR. I'll change the ticket description.

tstuefe

Good.

tstuefe · 2023-07-04T12:46:19Z

src/hotspot/share/utilities/ostream.cpp

@@ -186,18 +186,9 @@ void outputStream::put(char ch) {
  write(buf, 1);
 }

-#define SP_USE_TABS false
-
 void outputStream::sp(int count) {
  if (count < 0)  return;


Is this needed?

The count < 0 check? Yes, as Shipilev mentioned:

Looks okay. I thought what would happen if _indentation < _position, but sp() seems to handle count < 0 fine.

So it's nice that we don't have to care at the call site.

Ah, okay. Missed that.

dholmes-ora

Looks good.

As an aside I was disappointed to discover that the indentation facility is far less useful than it might be. I had hoped you could set the indentation and then have all following log output indented, until you reset it. But indentation only happens when you explicitly call indent() so you can't call e.g. val->print_on(stream) and have the caller control indentation (without print_on directly supporting it).

jdksjolen · 2023-07-07T09:23:35Z

One structured concurrency test fails on GHA on Linux-x86. That can't be thanks to this patch. Integrating.

/integrate

openjdk · 2023-07-07T09:24:32Z

Going to push as commit 92ca670.
Since your change was applied there have been 273 commits pushed to the master branch:

25cbe85: 8310550: Adjust references to rt.jar
848abd2: 8311511: Improve description of NativeLibrary JFR event
6485b7d: 6875229: Wrong placement of icons in JTabbedPane in Nimbus
27de536: 8311581: Remove obsolete code and comments in TestLVT.java
e848d94: 8310923: Refactor Currency tests to use JUnit
0c86c31: 8302351: "assert(!JavaThread::current()->is_interp_only_mode() || !nm->method()->is_continuation_enter_intrinsic() || ContinuationEntry::is_interpreted_call(return_pc)) failed: interp_only_mode but not in enterSpecial interpreted entry" in fixup_callers_callsite
ec7da91: 8240567: MethodTooLargeException thrown while creating a jlink image
97e99f0: 8311087: PhiNode::wait_for_region_igvn should break early
7173c30: 8307766: Linux: Provide the option to override the timer slack
356067d: 8311489: Remove unused dirent_md files
... and 263 more: https://git.openjdk.org/jdk/compare/955fc2fabafc4c4d79d137bf86194228d5cf8ad8...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-07-07T09:24:44Z

@jdksjolen Pushed as commit 92ca670.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Use the count argument for sp()

d9ee4c2

openjdk bot added the hotspot hotspot-dev@openjdk.org label Jun 15, 2023

jdksjolen marked this pull request as ready for review June 16, 2023 08:36

openjdk bot added rfr Pull request is ready for review hotspot-runtime hotspot-runtime-dev@openjdk.org labels Jun 16, 2023

shipilev approved these changes Jun 16, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 16, 2023

dholmes-ora approved these changes Jun 19, 2023

View reviewed changes

jdksjolen changed the title ~~8310170: Use sp's argument to improve performance of outputStream::indent~~ 8310170: Use sp's argument to improve performance of outputStream::indent and remove SP_USE_TABS Jul 4, 2023

Remove SP_USE_TABS

329e832

tstuefe approved these changes Jul 4, 2023

View reviewed changes

dholmes-ora approved these changes Jul 5, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 7, 2023

openjdk bot closed this Jul 7, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8310170: Use sp's argument to improve performance of outputStream::indent and remove SP_USE_TABS #14502

8310170: Use sp's argument to improve performance of outputStream::indent and remove SP_USE_TABS #14502

jdksjolen commented Jun 15, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 15, 2023

openjdk bot commented Jun 15, 2023

jdksjolen commented Jun 16, 2023

openjdk bot commented Jun 16, 2023

mlbridge bot commented Jun 16, 2023 •

edited

Loading

shipilev left a comment

openjdk bot commented Jun 16, 2023 •

edited

Loading

dholmes-ora left a comment

jdksjolen commented Jul 4, 2023

tstuefe left a comment

tstuefe Jul 4, 2023

jdksjolen Jul 4, 2023

tstuefe Jul 4, 2023

dholmes-ora left a comment

jdksjolen commented Jul 7, 2023

openjdk bot commented Jul 7, 2023

openjdk bot commented Jul 7, 2023

8310170: Use sp's argument to improve performance of outputStream::indent and remove SP_USE_TABS #14502

8310170: Use sp's argument to improve performance of outputStream::indent and remove SP_USE_TABS #14502

Conversation

jdksjolen commented Jun 15, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 15, 2023

openjdk bot commented Jun 15, 2023

jdksjolen commented Jun 16, 2023

openjdk bot commented Jun 16, 2023

mlbridge bot commented Jun 16, 2023 • edited Loading

Webrevs

shipilev left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 16, 2023 • edited Loading

dholmes-ora left a comment

Choose a reason for hiding this comment

jdksjolen commented Jul 4, 2023

tstuefe left a comment

Choose a reason for hiding this comment

tstuefe Jul 4, 2023

Choose a reason for hiding this comment

jdksjolen Jul 4, 2023

Choose a reason for hiding this comment

tstuefe Jul 4, 2023

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

jdksjolen commented Jul 7, 2023

openjdk bot commented Jul 7, 2023

openjdk bot commented Jul 7, 2023

jdksjolen commented Jun 15, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 16, 2023 •

edited

Loading

openjdk bot commented Jun 16, 2023 •

edited

Loading