Skip to content
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

flat_hash_map fails to compile when 'Warnings as Errors' is turned on in VS2017 #208

Closed
ChewyGumball opened this issue Oct 30, 2018 · 3 comments
Assignees

Comments

@ChewyGumball
Copy link

When I include absl/container/flat_hash_set.h or absl/container/flat_hash_map.h I get the following warnings:

absl\container\internal\raw_hash_set.h(206): warning C4244: 'argument': conversion from 'T' to 'uint32_t', possible loss of data
        with
        [
            T=uint64_t
        ]
absl\container\internal\raw_hash_set.h(431): note: see reference to function template instantiation 'int absl::container_internal::TrailingZeros<uint64_t>(T)' being compiled
        with
        [
            T=uint64_t
        ]
absl\container\internal\raw_hash_set.h(212): warning C4267: 'argument': conversion from 'size_t' to 'uint32_t', possible loss of data
absl\container\internal\raw_hash_set.h(480): note: see reference to function template instantiation 'int absl::container_internal::LeadingZeros<size_t>(T)' being compiled
        with
        [
            T=size_t
        ]

Looking at the TrailingZeros and LeadingZeros functions we see:

template <typename T>
int TrailingZeros(T x) {
  return sizeof(T) == 8 ? base_internal::CountTrailingZerosNonZero64(x)
                        : base_internal::CountTrailingZerosNonZero32(x);
}

template <typename T>
int LeadingZeros(T x) {
  return sizeof(T) == 8 ? base_internal::CountLeadingZeros64(x)
                        : base_internal::CountLeadingZeros32(x);
}

The warning is spurious because the base_internal call that won't lose data will be used, but the other is still compiled, and thus warned about.

In my case, I have turned on 'Warnings as Errors', so I can't even compile without static casting x to the type of the argument of CountLeadingZerosXX or CountTrailingZerosNonZero64.

@derekmauro derekmauro self-assigned this Nov 6, 2018
absl-federation-github pushed a commit that referenced this issue Nov 7, 2018
--
178e7a9a76fc8fcd6df6335b59139cbe644a16b9 by Jon Cohen <cohenjon@google.com>:

Import of CCTZ from GitHub.

PiperOrigin-RevId: 220523164

--
59ef14fe7034a3148f1e9cef1f128b8ca264b444 by Jon Cohen <cohenjon@google.com>:

Don't assume how much std::vector's constructors allocate in InlinedVector's test for scoped_allocator_adaptor support.

PiperOrigin-RevId: 220464683

--
6f8351be43a44a8f10bf20612b2cc744a4a911c7 by Jon Cohen <cohenjon@google.com>:

Add VS Code and some Bazel output files to absl/.gitignore

PiperOrigin-RevId: 220464362

--
43fac22f8af6b6ed55309a784a9d25d837393d0e by Abseil Team <absl-team@google.com>:

absl: fix SpinLock::EncodeWaitCycles

If a thread has ever observed or set kSpinLockSleeper, it must
never leave 0 in kWaitTimeMask because at this point it is
expected to wake subsequent threads. Current calculations in
EncodeWaitCycles can result in 0 in kWaitTimeMask and lead to
missed wakeups. This is mostly theoretical today, because
the futex call needs to finish within 128 cycles (futex can
return immediately without waiting, but 128 cycles still
look too low for this). But this can well fire in future
if we bump granularity and/or threshold for recording contention.

Use kSpinLockSleeper instead of 0.

PiperOrigin-RevId: 220463123

--
def9b7e3d45c99d68cc52a4429256116d7f421f2 by Abseil Team <absl-team@google.com>:

absl: optimize SpinLock::SlowLock

Currently we record contention even after the first initial spin.
This leads to several performance issues:
1. If we succeed in acquiring the lock after the initial spin,
overall wait time can be within tens/hundreds of nanoseconds.
Recording such low wait time looks completely unnecessary and excessive.
From some point of view this is not even a wait, because we did not sleep.
And, for example, Mutex does not record contention in this case.
In majority of cases the lock should be acquired exactly during the initial
spin, yet we still go through full overhead of submitting contention.
2. Whenever a thread submits contention it also calls FUTEX_WAKE
(there is no way to understand if it's necessary or not when wait value
is stored in the lock). So if there are just 2 threads and a brief
contention, the second thread will still call FUTEX_WAKE which
is completely unnecessary overhead.

Don't record contention after the initial spin wait.

FWIW this also removes 2 CycleClock::Now calls and EncodeWaitCycles
from the common hot path.

PiperOrigin-RevId: 220379972

--
75b0c0cb214de904ea622f81ec3f4eabdc8695b0 by Derek Mauro <dmauro@google.com>:

Supress MSVC warnings in raw_hash_set's use of TrailingZeros and LeadingZeros.
#208

PiperOrigin-RevId: 220372204
GitOrigin-RevId: 178e7a9a76fc8fcd6df6335b59139cbe644a16b9
Change-Id: I3a66af4e050810a3274e45d4e055b2aa19ffba1b
@derekmauro
Copy link
Member

I wasn't able to reproduce this (maybe I have a different version of MSVC?), but I added some casts that should fix this for you. Let us know if it doesn't.

@Bu11etmagnet
Copy link

This should be a compile-time decision, not a run-time decision, e.g. if constexpr (...).
Is there a constexpr ? : yet?

@derekmauro
Copy link
Member

derekmauro commented Nov 8, 2018

Since we are supporting C++11, we can't use if constexpr, but I'm pretty sure all modern compilers are smart enough to optimize away the constant branch in this case.

4s5t2os41n added a commit to 4s5t2os41n/abseil-cpp that referenced this issue Jul 4, 2024
--
178e7a9a76fc8fcd6df6335b59139cbe644a16b9 by Jon Cohen <cohenjon@google.com>:

Import of CCTZ from GitHub.

PiperOrigin-RevId: 220523164

--
59ef14fe7034a3148f1e9cef1f128b8ca264b444 by Jon Cohen <cohenjon@google.com>:

Don't assume how much std::vector's constructors allocate in InlinedVector's test for scoped_allocator_adaptor support.

PiperOrigin-RevId: 220464683

--
6f8351be43a44a8f10bf20612b2cc744a4a911c7 by Jon Cohen <cohenjon@google.com>:

Add VS Code and some Bazel output files to absl/.gitignore

PiperOrigin-RevId: 220464362

--
43fac22f8af6b6ed55309a784a9d25d837393d0e by Abseil Team <absl-team@google.com>:

absl: fix SpinLock::EncodeWaitCycles

If a thread has ever observed or set kSpinLockSleeper, it must
never leave 0 in kWaitTimeMask because at this point it is
expected to wake subsequent threads. Current calculations in
EncodeWaitCycles can result in 0 in kWaitTimeMask and lead to
missed wakeups. This is mostly theoretical today, because
the futex call needs to finish within 128 cycles (futex can
return immediately without waiting, but 128 cycles still
look too low for this). But this can well fire in future
if we bump granularity and/or threshold for recording contention.

Use kSpinLockSleeper instead of 0.

PiperOrigin-RevId: 220463123

--
def9b7e3d45c99d68cc52a4429256116d7f421f2 by Abseil Team <absl-team@google.com>:

absl: optimize SpinLock::SlowLock

Currently we record contention even after the first initial spin.
This leads to several performance issues:
1. If we succeed in acquiring the lock after the initial spin,
overall wait time can be within tens/hundreds of nanoseconds.
Recording such low wait time looks completely unnecessary and excessive.
From some point of view this is not even a wait, because we did not sleep.
And, for example, Mutex does not record contention in this case.
In majority of cases the lock should be acquired exactly during the initial
spin, yet we still go through full overhead of submitting contention.
2. Whenever a thread submits contention it also calls FUTEX_WAKE
(there is no way to understand if it's necessary or not when wait value
is stored in the lock). So if there are just 2 threads and a brief
contention, the second thread will still call FUTEX_WAKE which
is completely unnecessary overhead.

Don't record contention after the initial spin wait.

FWIW this also removes 2 CycleClock::Now calls and EncodeWaitCycles
from the common hot path.

PiperOrigin-RevId: 220379972

--
75b0c0cb214de904ea622f81ec3f4eabdc8695b0 by Derek Mauro <dmauro@google.com>:

Supress MSVC warnings in raw_hash_set's use of TrailingZeros and LeadingZeros.
abseil/abseil-cpp#208

PiperOrigin-RevId: 220372204
GitOrigin-RevId: 178e7a9a76fc8fcd6df6335b59139cbe644a16b9
Change-Id: I3a66af4e050810a3274e45d4e055b2aa19ffba1b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants