RFR: 8263718: unused-result warning happens at os_linux.cpp

[David Holmes]

Okay so this may all be moot :)

On 19/03/2021 3:09 pm, David Holmes wrote:
> On 19/03/2021 10:17 am, Yasumasa Suenaga wrote:
>> On Fri, 19 Mar 2021 00:02:21 GMT, Yasumasa Suenaga 
>> <ysuenaga at openjdk.org> wrote:
>>
>>>>> The alloca was added as a performance boost for hyperthreaded systems
>>>>> back in 2003 for JDK 5:
>>>>>
>>>>> "A per-thread offset was added to each thread's stack to randomize the
>>>>> cachelines of hot stack frames (aka, stack coloring)."
>>>>
>>>> IIUC it relates to share DTLB between two logical processors when 
>>>> Hyperthreading is enabled.
>>>>
>>>>> I'm running some of our benchmarks to see if removing it makes a
>>>>> difference ... but chances are I'm not going to be running it on the
>>>>> kind of machines for which it was introduced.
>>>>
>>>> Thanks!
>>>>
>>>>>> If we decide to remain this code, we need to avoid unused-result 
>>>>>> warning from GCC. I think we can use pragma because other pragmas 
>>>>>> (format-nonliteral, format-security, stringop-truncation") are 
>>>>>> used in HotSpot. In addition, this behavior does not seem to 
>>>>>> determine to be a bug in GCC (status is UNCONFIRMED). However I 
>>>>>> will agree to use `(void)!` if it is still preferred based on them.
>>>>>
>>>>> I'd reluctantly prefer to use the pragma as an official mechanism for
>>>>> avoiding the warning.
>>>>
>>>> Ok, OpenJDK folks who discuss in this PR are not prefer to use 
>>>> pragma, so I will use `(void)!` if it is needed.
>>>> Let's see the result of benchmark and discuss what should we do.
>>>
>>>>> The alloca was added as a performance boost for hyperthreaded systems
>>>>> back in 2003 for JDK 5:
>>>>> "A per-thread offset was added to each thread's stack to randomize the
>>>>> cachelines of hot stack frames (aka, stack coloring)."
>>>>
>>>> IIUC it relates to share DTLB between two logical processors when 
>>>> Hyperthreading is enabled.
>>>
>>> `alloca()` call might be less effective if ASLR is enabled. It can be 
>>> configured by 
>>> [randomize_va_space](https://www.kernel.org/doc/html/latest/admin-guide/sysctl/kernel.html#randomize-va-space), 
>>> and I guess it is enabled in a lot of x86 systems.
> 
> It relates to L1 cache index collisions primarily so only impacts 
> hyperthreading on Intel CPUs (it also impacted CMT on SPARC). I've been 
> told that ASLR may render the manual stack-coloring unnecessary (as well 
> as ineffective), but then we'd need to establish which platforms have 
> that enabled, and whether we can tell.
> 
> The benchmarking has been inconclusive - no observable differences. But 
> the benchmarking machines (and indeed the benchmarks) may not be 
> representative of the context where this optimization is needed.
> 
> It has also been raised (as it was here) whether the alloca is even left 
> in place by the compiler. Something I have yet to check.

	call	_ZN6Thread26record_stack_base_and_sizeEv at PLT
.LVL2492:
	.loc 3 666 3 is_stmt 1 view .LVU7882
	.loc 3 667 3 view .LVU7883
.LBB6399:
.LBI6399:
	.loc 3 1359 5 view .LVU7884
.LBB6400:
	.loc 3 1360 3 view .LVU7885
	.loc 3 1360 18 is_stmt 0 view .LVU7886
	call	getpid at PLT
.LVL2493:
.LBE6400:
.LBE6399:
	.loc 3 668 3 is_stmt 1 view .LVU7887
	.loc 3 670 36 is_stmt 0 view .LVU7888
	movq	%r13, %rdi
	.loc 3 668 3 view .LVU7889
	addl	$1, _ZZL19thread_native_entryP6ThreadE7counter(%rip)
	.loc 3 670 3 is_stmt 1 view .LVU7890
	.loc 3 670 36 is_stmt 0 view .LVU7891
	call	_ZN6Thread25initialize_thread_currentEv at PLT

Appears the alloca has been elided, along with the arithmetic.

Don't know if that is true for other platforms.

David
-----

>>> BTW this is not the first time we have had this issue with gcc making a
>>> function deprecated and casting to void not fixing it. Unfortunately I
>>> can't remember enough of the previous case's details to actually look it
>>> up and see what we resolved to do. :(
>>
>> I found 
>> [JDK-6879689](https://bugs.openjdk.java.net/browse/JDK-6879689), and 
>> it fixed unused-return as following:
>>
>> https://github.com/openjdk/jdk/blob/9b5a9b61899cf649104c0ff70e14549f64a89561/src/hotspot/share/adlc/archDesc.cpp#L1082-L1083 
>>
>>
>> I prefer it rather than `(void)!`.
> 
> But introducing a local (which was already suggested earlier) that is 
> unused in a product build may also trigger a warning from the compiler 
> and we are back where we started.
> 
> David
> 
>> -------------
>>
>> PR: https://git.openjdk.java.net/jdk/pull/3042
>>