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

[Thomas Stüfe]

On Fri, Mar 19, 2021 at 6:49 AM David Holmes <david.holmes at oracle.com>
wrote:

> 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
> -----
>
>
Which would explain why we don't see effects in benchmarks. Question is, do
we repair this or remove it.

..Thomas


> >>> 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
> >>
>