Skip to content

8236569: -Xss not multiple of 4K does not work for the main thread on macOS #4256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from

Conversation

slowhog
Copy link
Contributor

@slowhog slowhog commented May 28, 2021

…d on macOS

This patch simply round up the specified stack size to multiple of the system page size.

Test is trivial, simply run java with -Xss option against following code. On MacOS, before the fix, running with -Xss159k and -Xss160k would get 7183 and 649 respectively. After fix, both would output 649, while -Xss161k would be same as -Xss164k and see 691 as the output.

public class StackLeak {
    public int depth = 0;
    public void stackLeak() {
        depth++;
        stackLeak();
    }

    public static void main(String[] args) {
        var test = new StackLeak();
        try {
            test.stackLeak();
        } catch (Throwable e) {
            System.out.println(test.depth);
        }
    }
}

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8236569: -Xss not multiple of 4K does not work for the main thread on macOS

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256
$ git checkout pull/4256

Update a local copy of the PR:
$ git checkout pull/4256
$ git pull https://git.openjdk.java.net/jdk pull/4256/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4256

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4256.diff

@bridgekeeper
Copy link

bridgekeeper bot commented May 28, 2021

👋 Welcome back henryjen! 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 openjdk bot changed the title JDK-8236569: -Xss not multiple of 4K does not work for the main threa… 8236569: -Xss not multiple of 4K does not work for the main thread on macOS May 28, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label May 28, 2021
@openjdk
Copy link

openjdk bot commented May 28, 2021

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

  • core-libs

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.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label May 28, 2021
@mlbridge
Copy link

mlbridge bot commented May 28, 2021

Webrevs

@@ -722,6 +723,16 @@ static void MacOSXStartup(int argc, char *argv[]) {
return (void*)(intptr_t)JavaMain(args);
}

static size_t alignUp(size_t stack_size) {
long page_size = sysconf(_SC_PAGESIZE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hotspot we use getpagesize(). There is also a guard for a very large stack (within a page of SIZE_MAX) so that rounding up does not produce zero.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds like that (getpagesize) should work with m1 mac as well, as they have 16k pages. will it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sysconf is the portable way based on POSIX, we can use getpagesize give this is macOS specific code, which is BSD based.

@slowhog slowhog closed this Jun 3, 2021
@slowhog
Copy link
Contributor Author

slowhog commented Jun 3, 2021

Withdraw for keep current behavior for compatibility. It would be preferred for user to specify proper value than we change the value on user's behalf.

@slowhog slowhog reopened this Jun 4, 2021
@dholmes-ora
Copy link
Member

I'm a bit confused about the state of this PR. It was closed/withdrawn but then reopened but with changes applied to platforms other than macOS - why?

@dholmes-ora
Copy link
Member

Also the code does not compile on Linux in current form.

@slowhog
Copy link
Contributor Author

slowhog commented Jun 7, 2021

Planned to close JDK-8236569 as 'Won't Fix', as the issue was re-opened, we give it another shot. As explained in the CSR review, we will only round-up the stack size as required by the operating system. Test on Ubuntu shows that there is no need to round-up, while some other Posix system might as explained in the man page.
We round-up for MacOS as the document explicitly said that the size need to be multiple of system page size. I also changed to use getpagesize() as you suggested, although that's not needed.

@dholmes-ora
Copy link
Member

while some other Posix system might as explained in the man page

What manpage?

The POSIX specification for this does not allow for EINVAL being returned due to alignment issues. That is an extra constraint imposed by macOS and which makes it non-conforming to the POSIX spec IMO. While the changes in src/java.base/unix/native/libjli/java_md.c seem perfectly fine, are we actually ever going to execute it?

@slowhog
Copy link
Contributor Author

slowhog commented Jun 7, 2021

Linux man page for pthread_attr_setstacksize() states that,

   On some systems, pthread_attr_setstacksize() can fail with the
   error EINVAL if stacksize is not a multiple of the system page
   size.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Henry,

I'm okay with these changes in the current form.

The help text needs tweaking - see CSR request.

Thanks,
David

@@ -172,6 +172,8 @@ java.launcher.X.usage=\n\
\ (Linux Only) show host system or container\n\
\ configuration and continue\n\
\ -Xss<size> set java thread stack size\n\
\ The actual size may be round up to multiple of system\n\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See updated help text in the CSR request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, thanks

@openjdk
Copy link

openjdk bot commented Jun 8, 2021

@slowhog 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:

8236569: -Xss not multiple of 4K does not work for the main thread on macOS

Reviewed-by: dholmes, alanb

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 684 new commits pushed to the master branch:

  • f4b3ee5: 8270280: security/infra/java/security/cert/CertPathValidator/certification/LetsEncryptCA.java OCSP response error
  • 45abbee: 8243543: jtreg test security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java fails
  • c9251db: 8271209: Fix doc comment typos in JavadocTokenizer
  • 96247ae: 8270187: G1: Remove ConcGCThreads constraint
  • 9b27df6: 8271063: Print injected fields for InstanceKlass
  • 0cc4bb7: 8270870: Simplify G1ServiceThread
  • 8c8e3a0: 8271163: G1 uses wrong degree of MT processing since JDK-8270169
  • 8a789b7: 8263840: PeriodicTask should declare its destructor virtual
  • f226190: 8270894: Use acquire semantics in ObjectSynchronizer::read_stable_mark()
  • ea182b5: 8271060: Merge G1CollectedHeap::determine_start_concurrent_mark_gc and G1Policy::decide_on_conc_mark_initiation
  • ... and 674 more: https://git.openjdk.java.net/jdk/compare/52d88ee1d1e0f6b9927db03a2b0bff75e4be03a2...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 8, 2021
Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

proposed shorter form. Otherwise this looks fine.

Cheers, Thomas

}
return page_size * pages;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could probably be shortened to something like this:

size_t pagesize = (size_t)sysconf(_SC_PAGESIZE);
return (stack_size + (pagesize - 1)) & ~(pagesize - 1);

or, if you insist on checking for SIZE_MAX:

size_t pagesize = (size_t)sysconf(_SC_PAGESIZE);
size_t max = SIZE_MAX - pagesize;
return stack_size <= max ? (stack_size + (pagesize - 1)) & ~(pagesize - 1) : max;

(I see David requested this, so this is fine, though passing SIZE_MAX to this function will quite likely fail anyway :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While sysconf(_SC_PAGESIZE) is most likely(if not always) be power of 2, it's not a constant we know for sure here and this is not critical path for performance, thus I didn't take that approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern was not performance but brevity, especially since you add the same function twice. And about using the same logic for aligning up as we do within hotspot (see align.hpp). You also mix up size_t and long (signed vs unsigned, and potentially different sizes) - there is nothing obvious wrong with that but I would at least consistently use size_t here.

@tstuefe
Copy link
Member

tstuefe commented Jun 8, 2021

Please make sure the failing tests have nothing to do with your patch. gc/shenandoah/compiler/TestLinkToNativeRBP.java
sounds at least suggestive.

@mlbridge
Copy link

mlbridge bot commented Jun 9, 2021

Mailing list message from David Holmes on core-libs-dev:

On 8/06/2021 11:40 pm, Thomas Stuefe wrote:

On Mon, 7 Jun 2021 03:18:32 GMT, Henry Jen <henryjen at openjdk.org> wrote:

?d on macOS

This patch simply round up the specified stack size to multiple of the system page size.

Test is trivial, simply run java with -Xss option against following code. On MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get `7183` and `649` respectively. After fix, both would output `649`, while `-Xss161k` would be same as `-Xss164k` and see 691 as the output.

```code:java
public class StackLeak {
public int depth = 0;
public void stackLeak() {
depth++;
stackLeak();
}

 public static void main\(String\[\] args\) \{
     var test \= new StackLeak\(\)\;
     try \{
         test\.stackLeak\(\)\;
     \} catch \(Throwable e\) \{
         System\.out\.println\(test\.depth\)\;
     \}
 \}

}

Henry Jen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:

- Cast type
- Merge
- Change java -X output for -Xss
- Merge
- Only try to round-up when current value failed
- Avoid overflow on page size
- JDK-8236569: -Xss not multiple of 4K does not work for the main thread on macOS

Please make sure the failing tests have nothing to do with your patch. `gc/shenandoah/compiler/TestLinkToNativeRBP.java`
sounds at least suggestive.

warning: using incubating module(s): jdk.incubator.foreign
/home/runner/work/jdk/jdk/test/hotspot/jtreg/gc/shenandoah/compiler/TestLinkToNativeRBP.java:42:
error: cannot find symbol
import jdk.incubator.foreign.LibraryLookup;

Looks like shenandoah test was not updated for latest Foreign changes.

David

@mlbridge
Copy link

mlbridge bot commented Jun 9, 2021

Mailing list message from David Holmes on core-libs-dev:

On 8/06/2021 11:40 pm, Thomas Stuefe wrote:

On Mon, 7 Jun 2021 03:18:32 GMT, Henry Jen <henryjen at openjdk.org> wrote:

?d on macOS

This patch simply round up the specified stack size to multiple of the system page size.

Test is trivial, simply run java with -Xss option against following code. On MacOS, before the fix, running with `-Xss159k` and `-Xss160k` would get `7183` and `649` respectively. After fix, both would output `649`, while `-Xss161k` would be same as `-Xss164k` and see 691 as the output.

```code:java
public class StackLeak {
public int depth = 0;
public void stackLeak() {
depth++;
stackLeak();
}

 public static void main\(String\[\] args\) \{
     var test \= new StackLeak\(\)\;
     try \{
         test\.stackLeak\(\)\;
     \} catch \(Throwable e\) \{
         System\.out\.println\(test\.depth\)\;
     \}
 \}

}

Henry Jen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:

- Cast type
- Merge
- Change java -X output for -Xss
- Merge
- Only try to round-up when current value failed
- Avoid overflow on page size
- JDK-8236569: -Xss not multiple of 4K does not work for the main thread on macOS

Please make sure the failing tests have nothing to do with your patch. `gc/shenandoah/compiler/TestLinkToNativeRBP.java`
sounds at least suggestive.

warning: using incubating module(s): jdk.incubator.foreign
/home/runner/work/jdk/jdk/test/hotspot/jtreg/gc/shenandoah/compiler/TestLinkToNativeRBP.java:42:
error: cannot find symbol
import jdk.incubator.foreign.LibraryLookup;

Looks like shenandoah test was not updated for latest Foreign changes.

David

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 7, 2021

@slowhog This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@slowhog
Copy link
Contributor Author

slowhog commented Jul 14, 2021

Still pending CSR, also considering adapt hotspot align up as suggested by @tstuefe.


if (stack_size > 0) {
pthread_attr_setstacksize(&attr, stack_size);
if (EINVAL == pthread_attr_setstacksize(&attr, stack_size)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style-wise it might be more consistent put EINVAL on the RHS of the ==.

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 20, 2021

@slowhog This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 17, 2021

@slowhog This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Sep 17, 2021
@asotona
Copy link
Member

asotona commented May 31, 2022

/open

@openjdk
Copy link

openjdk bot commented May 31, 2022

@asotona Only the pull request author can set the pull request state to "open"

@asotona
Copy link
Member

asotona commented May 31, 2022

Continuation in #8953

@slowhog slowhog deleted the JDK-8236569 branch January 16, 2025 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org ready Pull request is ready to be integrated rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

6 participants