Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Tue, 8 Jun 2021 13:37:10 GMT, Thomas Stuefe wrote: >> 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. Still pending CSR, also considering adapt hotspot align up as suggested by @tstuefe. - PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Tue, 8 Jun 2021 16:45:12 GMT, Henry Jen wrote: >> src/java.base/unix/native/libjli/java_md.c line 666: >> >>> 664: return page_size * pages; >>> 665: } >>> 666: } >> >> 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 :) > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On 8/06/2021 11:40 pm, Thomas Stuefe wrote:
On Mon, 7 Jun 2021 03:18:32 GMT, Henry Jen 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
-
PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Tue, 8 Jun 2021 08:17:54 GMT, Thomas Stuefe wrote: >> 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 > > src/java.base/unix/native/libjli/java_md.c line 666: > >> 664: return page_size * pages; >> 665: } >> 666: } > > 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 :) 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. - PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Tue, 8 Jun 2021 02:36:26 GMT, David Holmes wrote: >> 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 > > src/java.base/share/classes/sun/launcher/resources/launcher.properties line > 175: > >> 173: \ configuration and continue\n\ >> 174: \-Xssset java thread stack size\n\ >> 175: \ The actual size may be round up to multiple of >> system\n\ > > See updated help text in the CSR request. Updated, thanks - PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Mon, 7 Jun 2021 03:18:32 GMT, Henry Jen 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.
-
PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Mon, 7 Jun 2021 03:18:32 GMT, Henry Jen 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
Hi,
proposed shorter form. Otherwise this looks fine.
Cheers, Thomas
src/java.base/unix/native/libjli/java_md.c line 666:
> 664: return page_size * pages;
> 665: }
> 666: }
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 :)
-
PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Mon, 7 Jun 2021 03:18:32 GMT, Henry Jen 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
Hi Henry,
I'm okay with these changes in the current form.
The help text needs tweaking - see CSR request.
Thanks,
David
src/java.base/share/classes/sun/launcher/resources/launcher.properties line 175:
> 173: \ configuration and continue\n\
> 174: \-Xssset java thread stack size\n\
> 175: \ The actual size may be round up to multiple of
> system\n\
See updated help text in the CSR request.
-
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Mon, 7 Jun 2021 03:18:32 GMT, Henry Jen 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
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.
-
PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Mon, 7 Jun 2021 03:22:18 GMT, Henry Jen wrote: > 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? - PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
On Mon, 7 Jun 2021 03:18:32 GMT, Henry Jen 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
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.
-
PR: https://git.openjdk.java.net/jdk/pull/4256
Re: RFR: 8236569: -Xss not multiple of 4K does not work for the main thread on macOS [v5]
> …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
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4256/files
- new: https://git.openjdk.java.net/jdk/pull/4256/files/764a1f93..5a8d4a10
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=4256=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk=4256=03-04
Stats: 1679 lines in 41 files changed: 1388 ins; 168 del; 123 mod
Patch: https://git.openjdk.java.net/jdk/pull/4256.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4256/head:pull/4256
PR: https://git.openjdk.java.net/jdk/pull/4256
