From c069af240b7197cf1b968df29b4206597d719c22 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Fri, 30 Mar 2018 20:11:56 -0700 Subject: [PATCH 01/13] Fix grpc_millis type (timers are broken on 32-bit systems otherwise) --- src/core/lib/iomgr/exec_ctx.cc | 18 +++++++++--------- src/core/lib/iomgr/exec_ctx.h | 6 +++--- src/core/lib/iomgr/timer.h | 2 +- src/core/lib/iomgr/timer_generic.cc | 5 +++-- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/core/lib/iomgr/exec_ctx.cc b/src/core/lib/iomgr/exec_ctx.cc index 2f544b20ab9..5d5c355ff98 100644 --- a/src/core/lib/iomgr/exec_ctx.cc +++ b/src/core/lib/iomgr/exec_ctx.cc @@ -53,24 +53,24 @@ static void exec_ctx_sched(grpc_closure* closure, grpc_error* error) { static gpr_timespec g_start_time; -static gpr_atm timespec_to_atm_round_down(gpr_timespec ts) { +static grpc_millis timespec_to_millis_round_down(gpr_timespec ts) { ts = gpr_time_sub(ts, g_start_time); double x = GPR_MS_PER_SEC * static_cast(ts.tv_sec) + static_cast(ts.tv_nsec) / GPR_NS_PER_MS; if (x < 0) return 0; - if (x > GPR_ATM_MAX) return GPR_ATM_MAX; - return static_cast(x); + if (x > GRPC_MILLIS_INF_FUTURE) return GRPC_MILLIS_INF_FUTURE; + return static_cast(x); } -static gpr_atm timespec_to_atm_round_up(gpr_timespec ts) { +static grpc_millis timespec_to_millis_round_up(gpr_timespec ts) { ts = gpr_time_sub(ts, g_start_time); double x = GPR_MS_PER_SEC * static_cast(ts.tv_sec) + static_cast(ts.tv_nsec) / GPR_NS_PER_MS + static_cast(GPR_NS_PER_SEC - 1) / static_cast(GPR_NS_PER_SEC); if (x < 0) return 0; - if (x > GPR_ATM_MAX) return GPR_ATM_MAX; - return static_cast(x); + if (x > GRPC_MILLIS_INF_FUTURE) return GRPC_MILLIS_INF_FUTURE; + return static_cast(x); } gpr_timespec grpc_millis_to_timespec(grpc_millis millis, @@ -92,12 +92,12 @@ gpr_timespec grpc_millis_to_timespec(grpc_millis millis, } grpc_millis grpc_timespec_to_millis_round_down(gpr_timespec ts) { - return timespec_to_atm_round_down( + return timespec_to_millis_round_down( gpr_convert_clock_type(ts, g_start_time.clock_type)); } grpc_millis grpc_timespec_to_millis_round_up(gpr_timespec ts) { - return timespec_to_atm_round_up( + return timespec_to_millis_round_up( gpr_convert_clock_type(ts, g_start_time.clock_type)); } @@ -138,7 +138,7 @@ bool ExecCtx::Flush() { grpc_millis ExecCtx::Now() { if (!now_is_valid_) { - now_ = timespec_to_atm_round_down(gpr_now(GPR_CLOCK_MONOTONIC)); + now_ = timespec_to_millis_round_down(gpr_now(GPR_CLOCK_MONOTONIC)); now_is_valid_ = true; } return now_; diff --git a/src/core/lib/iomgr/exec_ctx.h b/src/core/lib/iomgr/exec_ctx.h index 72d0ae58c10..4cc5586aa48 100644 --- a/src/core/lib/iomgr/exec_ctx.h +++ b/src/core/lib/iomgr/exec_ctx.h @@ -28,10 +28,10 @@ #include "src/core/lib/gpr/tls.h" #include "src/core/lib/iomgr/closure.h" -typedef gpr_atm grpc_millis; +typedef int64_t grpc_millis; -#define GRPC_MILLIS_INF_FUTURE GPR_ATM_MAX -#define GRPC_MILLIS_INF_PAST GPR_ATM_MIN +#define GRPC_MILLIS_INF_FUTURE INT64_MAX +#define GRPC_MILLIS_INF_PAST INT64_MIN /** A workqueue represents a list of work to be executed asynchronously. Forward declared here to avoid a circular dependency with workqueue.h. */ diff --git a/src/core/lib/iomgr/timer.h b/src/core/lib/iomgr/timer.h index 5ff10d3aee2..7f534476df2 100644 --- a/src/core/lib/iomgr/timer.h +++ b/src/core/lib/iomgr/timer.h @@ -28,7 +28,7 @@ #include "src/core/lib/iomgr/iomgr.h" typedef struct grpc_timer { - gpr_atm deadline; + grpc_millis deadline; uint32_t heap_index; /* INVALID_HEAP_INDEX if not in heap */ bool pending; struct grpc_timer* next; diff --git a/src/core/lib/iomgr/timer_generic.cc b/src/core/lib/iomgr/timer_generic.cc index 93e654b7fac..7a2767aa0c9 100644 --- a/src/core/lib/iomgr/timer_generic.cc +++ b/src/core/lib/iomgr/timer_generic.cc @@ -30,6 +30,7 @@ #include #include +#include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/debug/trace.h" #include "src/core/lib/gpr/spinlock.h" #include "src/core/lib/gpr/tls.h" @@ -337,7 +338,7 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, if (grpc_timer_trace.enabled()) { gpr_log(GPR_DEBUG, - "TIMER %p: SET %" PRIdPTR " now %" PRIdPTR " call %p[%p]", timer, + "TIMER %p: SET %" PRIdPTR " now %" PRId64 " call %p[%p]", timer, deadline, grpc_core::ExecCtx::Get()->Now(), closure, closure->cb); } @@ -650,7 +651,7 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { if (next == nullptr) { next_str = gpr_strdup("NULL"); } else { - gpr_asprintf(&next_str, "%" PRIdPTR, *next); + gpr_asprintf(&next_str, "%" PRId64, *next); } gpr_log(GPR_DEBUG, "TIMER CHECK END: r=%d; next=%s", r, next_str); gpr_free(next_str); From 9c142c9dc977062e7f87c29d27248421e1a5a2e7 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Tue, 10 Apr 2018 11:43:07 -0700 Subject: [PATCH 02/13] 64-bit atomic operations --- include/grpc/impl/codegen/atm64.h | 71 ++++++++++++++++++++ include/grpc/impl/codegen/atm64_gcc_atomic.h | 42 ++++++++++++ include/grpc/impl/codegen/atm64_gcc_sync.h | 43 ++++++++++++ include/grpc/impl/codegen/atm64_windows.h | 50 ++++++++++++++ include/grpc/support/atm64.h | 26 +++++++ src/core/lib/iomgr/exec_ctx.h | 7 +- src/core/lib/iomgr/timer_generic.cc | 65 +++++++++--------- 7 files changed, 269 insertions(+), 35 deletions(-) create mode 100644 include/grpc/impl/codegen/atm64.h create mode 100644 include/grpc/impl/codegen/atm64_gcc_atomic.h create mode 100644 include/grpc/impl/codegen/atm64_gcc_sync.h create mode 100644 include/grpc/impl/codegen/atm64_windows.h create mode 100644 include/grpc/support/atm64.h diff --git a/include/grpc/impl/codegen/atm64.h b/include/grpc/impl/codegen/atm64.h new file mode 100644 index 00000000000..86b0cc36272 --- /dev/null +++ b/include/grpc/impl/codegen/atm64.h @@ -0,0 +1,71 @@ +/* + * + * Copyright 2018 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#ifndef GRPC_IMPL_CODEGEN_ATM64_H +#define GRPC_IMPL_CODEGEN_ATM64_H + +/** This interface provides atomic operations and barriers for 64 bit integer + data types (instead of intptr_t so that this works on both 32-bit and 64-bit + systems. + + It is internal to gpr support code and should not be used outside it. + + If an operation with acquire semantics precedes another memory access by the + same thread, the operation will precede that other access as seen by other + threads. + + If an operation with release semantics follows another memory access by the + same thread, the operation will follow that other access as seen by other + threads. + + Routines with "acq" or "full" in the name have acquire semantics. Routines + with "rel" or "full" in the name have release semantics. Routines with + "no_barrier" in the name have neither acquire not release semantics. + + The routines may be implemented as macros. + + // Atomic operations act on an intergral_type gpr_atm64 that is 64 bit wide + typedef int64_t gpr_atm64; + + gpr_atm64 gpr_atm64_no_barrier_load(gpr_atm64 *p); + + // Atomically set *p = value, with release semantics. + void gpr_atm64_no_barrier_store(gpr_atm64 *p, gpr_atm64 value); +*/ + +#include + +#if defined(GPR_GCC_ATOMIC) +#include +#elif defined(GPR_GCC_SYNC) +#include +#elif defined(GPR_WINDOWS_ATOMIC) +#include +#else +#error could not determine platform for atm +#endif + +#ifdef __cplusplus +extern "C" { +#endif + +#ifdef __cplusplus +} +#endif + +#endif /* GRPC_IMPL_CODEGEN_ATM64_H */ diff --git a/include/grpc/impl/codegen/atm64_gcc_atomic.h b/include/grpc/impl/codegen/atm64_gcc_atomic.h new file mode 100644 index 00000000000..164002c752a --- /dev/null +++ b/include/grpc/impl/codegen/atm64_gcc_atomic.h @@ -0,0 +1,42 @@ +/* + * + * Copyright 2018 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#ifndef GRPC_IMPL_CODEGEN_ATM64_GCC_ATOMIC_H +#define GRPC_IMPL_CODEGEN_ATM64_GCC_ATOMIC_H + +/* atm_platform.h for gcc and gcc-like compilers with the + __atomic_* interface. */ +#include + +#ifdef __cplusplus +extern "C" { +#endif + +typedef int64_t gpr_atm64; +#define GPR_ATM64_MAX INT64_MAX +#define GPR_ATM64_MIN INT64_MIN + +#define gpr_atm_no_barrier_load(p) (__atomic_load_n((p), __ATOMIC_RELAXED)) +#define gpr_atm_no_barrier_store(p, value) \ + (__atomic_store_n((p), (int64_t)(value), __ATOMIC_RELAXED)) + +#ifdef __cplusplus +} +#endif + +#endif /* GRPC_IMPL_CODEGEN_ATM64_GCC_ATOMIC_H */ diff --git a/include/grpc/impl/codegen/atm64_gcc_sync.h b/include/grpc/impl/codegen/atm64_gcc_sync.h new file mode 100644 index 00000000000..f1fcfc89e88 --- /dev/null +++ b/include/grpc/impl/codegen/atm64_gcc_sync.h @@ -0,0 +1,43 @@ +/* + * + * Copyright 2015 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#ifndef GRPC_IMPL_CODEGEN_ATM64_GCC_SYNC64_H +#define GRPC_IMPL_CODEGEN_ATM64_GCC_SYNC64_H + +/* variant of atm_platform.h for gcc and gcc-like compiers with __sync_* + interface */ +#include + +typedef int64_t gpr_atm64; +#define GPR_ATM64_MAX INT64_MAX +#define GPR_ATM64_MIN INT64_MIN + +#define GPR_ATM64_COMPILE_BARRIER_() __asm__ __volatile__("" : : : "memory") + +static __inline gpr_atm64 gpr_atm64_no_barrier_load(const gpr_atm64* p) { + gpr_atm64 value = *p; + GPR_ATM64_COMPILE_BARRIER_(); + return value; +} + +static __inline void gpr_atm64_no_barrier_store(gpr_atm64* p, gpr_atm64 value) { + GPR_ATM64_COMPILE_BARRIER_(); + *p = value; +} + +#endif /* GRPC_IMPL_CODEGEN_ATM64_GCC_SYNC64_H */ diff --git a/include/grpc/impl/codegen/atm64_windows.h b/include/grpc/impl/codegen/atm64_windows.h new file mode 100644 index 00000000000..df5dd6f92cb --- /dev/null +++ b/include/grpc/impl/codegen/atm64_windows.h @@ -0,0 +1,50 @@ +/* + * + * Copyright 2015 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#ifndef GRPC_IMPL_CODEGEN_ATM64_WINDOWS_H +#define GRPC_IMPL_CODEGEN_ATM64_WINDOWS_H + +/** Win32 variant of atm_platform.h */ +#include + +typedef int64_t gpr_atm64; +#define GPR_ATM64_MAX INT64_MAX +#define GPR_ATM64_MIN INT64_MIN + +#define gpr_atm64_full_barrier MemoryBarrier + +static __inline gpr_atm64 gpr_atm64_acq_load(const gpr_atm64* p) { + gpr_atm64 result = *p; + gpr_atm64_full_barrier(); + return result; +} + +static __inline gpr_atm64 gpr_atm64_no_barrier_load(const gpr_atm64* p) { + return gpr_atm64_acq_load(p); +} + +static __inline void gpr_atm64_rel_store(gpr_atm64* p, gpr_atm64 value) { + gpr_atm64_full_barrier(); + *p = value; +} + +static __inline void gpr_atm64_no_barrier_store(gpr_atm64* p, gpr_atm64 value) { + gpr_atm64_rel_store(p, value); +} + +#endif /* GRPC_IMPL_CODEGEN_ATM64_WINDOWS_H */ diff --git a/include/grpc/support/atm64.h b/include/grpc/support/atm64.h new file mode 100644 index 00000000000..df5954656c0 --- /dev/null +++ b/include/grpc/support/atm64.h @@ -0,0 +1,26 @@ +/* + * + * Copyright 2018 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#ifndef GRPC_SUPPORT_ATM64_H +#define GRPC_SUPPORT_ATM64_H + +#include + +#include + +#endif /* GRPC_SUPPORT_ATM64_H */ diff --git a/src/core/lib/iomgr/exec_ctx.h b/src/core/lib/iomgr/exec_ctx.h index 4cc5586aa48..47d257d7050 100644 --- a/src/core/lib/iomgr/exec_ctx.h +++ b/src/core/lib/iomgr/exec_ctx.h @@ -22,16 +22,17 @@ #include #include +#include #include #include #include "src/core/lib/gpr/tls.h" #include "src/core/lib/iomgr/closure.h" -typedef int64_t grpc_millis; +typedef gpr_atm64 grpc_millis; -#define GRPC_MILLIS_INF_FUTURE INT64_MAX -#define GRPC_MILLIS_INF_PAST INT64_MIN +#define GRPC_MILLIS_INF_FUTURE GPR_ATM64_MAX +#define GRPC_MILLIS_INF_PAST GPR_ATM64_MIN /** A workqueue represents a list of work to be executed asynchronously. Forward declared here to avoid a circular dependency with workqueue.h. */ diff --git a/src/core/lib/iomgr/timer_generic.cc b/src/core/lib/iomgr/timer_generic.cc index 7a2767aa0c9..bf9a0593c6f 100644 --- a/src/core/lib/iomgr/timer_generic.cc +++ b/src/core/lib/iomgr/timer_generic.cc @@ -30,11 +30,11 @@ #include #include -#include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/debug/trace.h" #include "src/core/lib/gpr/spinlock.h" #include "src/core/lib/gpr/tls.h" #include "src/core/lib/gpr/useful.h" +#include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/iomgr/time_averaged_stats.h" #include "src/core/lib/iomgr/timer_heap.h" @@ -60,9 +60,9 @@ typedef struct { gpr_mu mu; grpc_time_averaged_stats stats; /* All and only timers with deadlines <= this will be in the heap. */ - gpr_atm queue_deadline_cap; + grpc_millis queue_deadline_cap; /* The deadline of the next timer due in this shard */ - gpr_atm min_deadline; + grpc_millis min_deadline; /* Index of this timer_shard in the g_shard_queue */ uint32_t shard_queue_index; /* This holds all timers with deadlines < queue_deadline_cap. Timers in this @@ -210,7 +210,7 @@ GPR_TLS_DECL(g_last_seen_min_timer); struct shared_mutables { /* The deadline of the next timer due across all timer shards */ - gpr_atm min_timer; + grpc_millis min_timer; /* Allow only one run_some_expired_timers at once */ gpr_spinlock checker_mu; bool initialized; @@ -220,18 +220,18 @@ struct shared_mutables { static struct shared_mutables g_shared_mutables; -static gpr_atm saturating_add(gpr_atm a, gpr_atm b) { - if (a > GPR_ATM_MAX - b) { - return GPR_ATM_MAX; +static grpc_millis saturating_add(grpc_millis a, grpc_millis b) { + if (a > GRPC_MILLIS_INF_FUTURE - b) { + return GRPC_MILLIS_INF_FUTURE; } return a + b; } -static grpc_timer_check_result run_some_expired_timers(gpr_atm now, - gpr_atm* next, +static grpc_timer_check_result run_some_expired_timers(grpc_millis now, + grpc_millis* next, grpc_error* error); -static gpr_atm compute_min_deadline(timer_shard* shard) { +static grpc_millis compute_min_deadline(timer_shard* shard) { return grpc_timer_heap_is_empty(&shard->heap) ? saturating_add(shard->queue_deadline_cap, 1) : grpc_timer_heap_top(&shard->heap)->deadline; @@ -337,9 +337,9 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, #endif if (grpc_timer_trace.enabled()) { - gpr_log(GPR_DEBUG, - "TIMER %p: SET %" PRIdPTR " now %" PRId64 " call %p[%p]", timer, - deadline, grpc_core::ExecCtx::Get()->Now(), closure, closure->cb); + gpr_log(GPR_DEBUG, "TIMER %p: SET %" PRIdPTR " now %" PRId64 " call %p[%p]", + timer, deadline, grpc_core::ExecCtx::Get()->Now(), closure, + closure->cb); } if (!g_shared_mutables.initialized) { @@ -374,7 +374,7 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, } if (grpc_timer_trace.enabled()) { gpr_log(GPR_DEBUG, - " .. add to shard %d with queue_deadline_cap=%" PRIdPTR + " .. add to shard %d with queue_deadline_cap=%" PRId64 " => is_first_timer=%s", static_cast(shard - g_shards), shard->queue_deadline_cap, is_first_timer ? "true" : "false"); @@ -395,11 +395,11 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, if (is_first_timer) { gpr_mu_lock(&g_shared_mutables.mu); if (grpc_timer_trace.enabled()) { - gpr_log(GPR_DEBUG, " .. old shard min_deadline=%" PRIdPTR, + gpr_log(GPR_DEBUG, " .. old shard min_deadline=%" PRId64, shard->min_deadline); } if (deadline < shard->min_deadline) { - gpr_atm old_min_deadline = g_shard_queue[0]->min_deadline; + grpc_millis old_min_deadline = g_shard_queue[0]->min_deadline; shard->min_deadline = deadline; note_deadline_change(shard); if (shard->shard_queue_index == 0 && deadline < old_min_deadline) { @@ -450,7 +450,7 @@ static void timer_cancel(grpc_timer* timer) { 'queue_deadline_cap') into into shard->heap. Returns 'true' if shard->heap has atleast ONE element REQUIRES: shard->mu locked */ -static int refill_heap(timer_shard* shard, gpr_atm now) { +static int refill_heap(timer_shard* shard, grpc_millis now) { /* Compute the new queue window width and bound by the limits: */ double computed_deadline_delta = grpc_time_averaged_stats_update_average(&shard->stats) * @@ -463,10 +463,10 @@ static int refill_heap(timer_shard* shard, gpr_atm now) { /* Compute the new cap and put all timers under it into the queue: */ shard->queue_deadline_cap = saturating_add(GPR_MAX(now, shard->queue_deadline_cap), - static_cast(deadline_delta * 1000.0)); + static_cast(deadline_delta * 1000.0)); if (grpc_timer_check_trace.enabled()) { - gpr_log(GPR_DEBUG, " .. shard[%d]->queue_deadline_cap --> %" PRIdPTR, + gpr_log(GPR_DEBUG, " .. shard[%d]->queue_deadline_cap --> %" PRId64, static_cast(shard - g_shards), shard->queue_deadline_cap); } for (timer = shard->list.next; timer != &shard->list; timer = next) { @@ -474,7 +474,7 @@ static int refill_heap(timer_shard* shard, gpr_atm now) { if (timer->deadline < shard->queue_deadline_cap) { if (grpc_timer_check_trace.enabled()) { - gpr_log(GPR_DEBUG, " .. add timer with deadline %" PRIdPTR " to heap", + gpr_log(GPR_DEBUG, " .. add timer with deadline %" PRId64 " to heap", timer->deadline); } list_remove(timer); @@ -487,7 +487,7 @@ static int refill_heap(timer_shard* shard, gpr_atm now) { /* This pops the next non-cancelled timer with deadline <= now from the queue, or returns NULL if there isn't one. REQUIRES: shard->mu locked */ -static grpc_timer* pop_one(timer_shard* shard, gpr_atm now) { +static grpc_timer* pop_one(timer_shard* shard, grpc_millis now) { grpc_timer* timer; for (;;) { if (grpc_timer_check_trace.enabled()) { @@ -518,8 +518,8 @@ static grpc_timer* pop_one(timer_shard* shard, gpr_atm now) { } /* REQUIRES: shard->mu unlocked */ -static size_t pop_timers(timer_shard* shard, gpr_atm now, - gpr_atm* new_min_deadline, grpc_error* error) { +static size_t pop_timers(timer_shard* shard, grpc_millis now, + grpc_millis* new_min_deadline, grpc_error* error) { size_t n = 0; grpc_timer* timer; gpr_mu_lock(&shard->mu); @@ -537,12 +537,12 @@ static size_t pop_timers(timer_shard* shard, gpr_atm now, return n; } -static grpc_timer_check_result run_some_expired_timers(gpr_atm now, - gpr_atm* next, +static grpc_timer_check_result run_some_expired_timers(grpc_millis now, + grpc_millis* next, grpc_error* error) { grpc_timer_check_result result = GRPC_TIMERS_NOT_CHECKED; - gpr_atm min_timer = gpr_atm_no_barrier_load(&g_shared_mutables.min_timer); + grpc_millis min_timer = gpr_atm_no_barrier_load(&g_shared_mutables.min_timer); gpr_tls_set(&g_last_seen_min_timer, min_timer); if (now < min_timer) { if (next != nullptr) *next = GPR_MIN(*next, min_timer); @@ -554,14 +554,15 @@ static grpc_timer_check_result run_some_expired_timers(gpr_atm now, result = GRPC_TIMERS_CHECKED_AND_EMPTY; if (grpc_timer_check_trace.enabled()) { - gpr_log(GPR_DEBUG, " .. shard[%d]->min_deadline = %" PRIdPTR, + gpr_log(GPR_DEBUG, " .. shard[%d]->min_deadline = %" PRId64, static_cast(g_shard_queue[0] - g_shards), g_shard_queue[0]->min_deadline); } while (g_shard_queue[0]->min_deadline < now || - (now != GPR_ATM_MAX && g_shard_queue[0]->min_deadline == now)) { - gpr_atm new_min_deadline; + (now != GRPC_MILLIS_INF_FUTURE && + g_shard_queue[0]->min_deadline == now)) { + grpc_millis new_min_deadline; /* For efficiency, we pop as many available timers as we can from the shard. This may violate perfect timer deadline ordering, but that @@ -573,8 +574,8 @@ static grpc_timer_check_result run_some_expired_timers(gpr_atm now, if (grpc_timer_check_trace.enabled()) { gpr_log(GPR_DEBUG, " .. result --> %d" - ", shard[%d]->min_deadline %" PRIdPTR " --> %" PRIdPTR - ", now=%" PRIdPTR, + ", shard[%d]->min_deadline %" PRId64 " --> %" PRId64 + ", now=%" PRId64, result, static_cast(g_shard_queue[0] - g_shards), g_shard_queue[0]->min_deadline, new_min_deadline, now); } @@ -616,7 +617,7 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { } if (grpc_timer_check_trace.enabled()) { gpr_log(GPR_DEBUG, - "TIMER CHECK SKIP: now=%" PRIdPTR " min_timer=%" PRIdPTR, now, + "TIMER CHECK SKIP: now=%" PRId64" min_timer=%" PRId64, now, min_timer); } return GRPC_TIMERS_CHECKED_AND_EMPTY; From a1b1095fc3f7807dc24f5aaeeae1147246406f6c Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Tue, 10 Apr 2018 15:10:04 -0700 Subject: [PATCH 03/13] use threadlocal variable optimization only on 64 bit machines --- src/core/lib/iomgr/timer_generic.cc | 45 ++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/src/core/lib/iomgr/timer_generic.cc b/src/core/lib/iomgr/timer_generic.cc index bf9a0593c6f..8b98e015a48 100644 --- a/src/core/lib/iomgr/timer_generic.cc +++ b/src/core/lib/iomgr/timer_generic.cc @@ -202,11 +202,19 @@ static void validate_non_pending_timer(grpc_timer* t) { #endif +#if GPR_ARCH_64 +/* NOTE: TODO(sreek) - Currently the thread local storage support in grpc is + for intptr_t which means on 32-bit machines it is not wide enough to hold + grpc_millis which is 64-bit. Adding thread local support for 64 bit values + is a lot of work for very little gain. So we are currently restricting this + optimization to only 64 bit machines */ + /* Thread local variable that stores the deadline of the next timer the thread * has last-seen. This is an optimization to prevent the thread from checking * shared_mutables.min_timer (which requires acquiring shared_mutables.mu lock, * an expensive operation) */ GPR_TLS_DECL(g_last_seen_min_timer); +#endif struct shared_mutables { /* The deadline of the next timer due across all timer shards */ @@ -250,8 +258,11 @@ static void timer_list_init() { g_shared_mutables.checker_mu = GPR_SPINLOCK_INITIALIZER; gpr_mu_init(&g_shared_mutables.mu); g_shared_mutables.min_timer = grpc_core::ExecCtx::Get()->Now(); + +#if GPR_ARCH_64 gpr_tls_init(&g_last_seen_min_timer); gpr_tls_set(&g_last_seen_min_timer, 0); +#endif for (i = 0; i < g_num_shards; i++) { timer_shard* shard = &g_shards[i]; @@ -280,7 +291,11 @@ static void timer_list_shutdown() { grpc_timer_heap_destroy(&shard->heap); } gpr_mu_destroy(&g_shared_mutables.mu); + +#if GPR_ARCH_64 gpr_tls_destroy(&g_last_seen_min_timer); +#endif + gpr_free(g_shards); gpr_free(g_shard_queue); g_shared_mutables.initialized = false; @@ -337,7 +352,7 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, #endif if (grpc_timer_trace.enabled()) { - gpr_log(GPR_DEBUG, "TIMER %p: SET %" PRIdPTR " now %" PRId64 " call %p[%p]", + gpr_log(GPR_DEBUG, "TIMER %p: SET %" PRId64 " now %" PRId64 " call %p[%p]", timer, deadline, grpc_core::ExecCtx::Get()->Now(), closure, closure->cb); } @@ -412,8 +427,10 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, } static void timer_consume_kick(void) { - /* force re-evaluation of last seeen min */ +#if GPR_ARCH_64 + /* Force re-evaluation of last seen min */ gpr_tls_set(&g_last_seen_min_timer, 0); +#endif } static void timer_cancel(grpc_timer* timer) { @@ -502,12 +519,12 @@ static grpc_timer* pop_one(timer_shard* shard, grpc_millis now) { timer = grpc_timer_heap_top(&shard->heap); if (grpc_timer_check_trace.enabled()) { gpr_log(GPR_DEBUG, - " .. check top timer deadline=%" PRIdPTR " now=%" PRIdPTR, + " .. check top timer deadline=%" PRId64 " now=%" PRId64, timer->deadline, now); } if (timer->deadline > now) return nullptr; if (grpc_timer_trace.enabled()) { - gpr_log(GPR_DEBUG, "TIMER %p: FIRE %" PRIdPTR "ms late via %s scheduler", + gpr_log(GPR_DEBUG, "TIMER %p: FIRE %" PRId64 "ms late via %s scheduler", timer, now - timer->deadline, timer->closure->scheduler->vtable->name); } @@ -543,7 +560,9 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, grpc_timer_check_result result = GRPC_TIMERS_NOT_CHECKED; grpc_millis min_timer = gpr_atm_no_barrier_load(&g_shared_mutables.min_timer); +#if GPR_ARCH_64 gpr_tls_set(&g_last_seen_min_timer, min_timer); +#endif if (now < min_timer) { if (next != nullptr) *next = GPR_MIN(*next, min_timer); return GRPC_TIMERS_CHECKED_AND_EMPTY; @@ -608,17 +627,21 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { // prelude grpc_millis now = grpc_core::ExecCtx::Get()->Now(); +#if GPR_ARCH_64 /* fetch from a thread-local first: this avoids contention on a globally mutable cacheline in the common case */ grpc_millis min_timer = gpr_tls_get(&g_last_seen_min_timer); +#else + grpc_millis min_timer = gpr_atm_no_barrier_load(&g_shared_mutables.min_timer); +#endif + if (now < min_timer) { if (next != nullptr) { *next = GPR_MIN(*next, min_timer); } if (grpc_timer_check_trace.enabled()) { - gpr_log(GPR_DEBUG, - "TIMER CHECK SKIP: now=%" PRId64" min_timer=%" PRId64, now, - min_timer); + gpr_log(GPR_DEBUG, "TIMER CHECK SKIP: now=%" PRId64 " min_timer=%" PRId64, + now, min_timer); } return GRPC_TIMERS_CHECKED_AND_EMPTY; } @@ -634,12 +657,12 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { if (next == nullptr) { next_str = gpr_strdup("NULL"); } else { - gpr_asprintf(&next_str, "%" PRIdPTR, *next); + gpr_asprintf(&next_str, "%" PRId64, *next); } gpr_log(GPR_DEBUG, - "TIMER CHECK BEGIN: now=%" PRIdPTR " next=%s tls_min=%" PRIdPTR - " glob_min=%" PRIdPTR, - now, next_str, gpr_tls_get(&g_last_seen_min_timer), + "TIMER CHECK BEGIN: now=%" PRId64 " next=%s tls_min=%" PRId64 + " glob_min=%" PRId64, + now, next_str, min_timer, gpr_atm_no_barrier_load(&g_shared_mutables.min_timer)); gpr_free(next_str); } From 6997718c6fb3b276762967f87a32e06c6495c2e3 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 11 Apr 2018 14:30:26 -0700 Subject: [PATCH 04/13] Change shared_mutables.min_timer to gpr_atm64 (Forgot to do this previously) --- include/grpc/impl/codegen/atm64_gcc_atomic.h | 4 ++-- src/core/lib/iomgr/timer_generic.cc | 14 ++++++++------ 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/include/grpc/impl/codegen/atm64_gcc_atomic.h b/include/grpc/impl/codegen/atm64_gcc_atomic.h index 164002c752a..f6dc7c6b8b2 100644 --- a/include/grpc/impl/codegen/atm64_gcc_atomic.h +++ b/include/grpc/impl/codegen/atm64_gcc_atomic.h @@ -31,8 +31,8 @@ typedef int64_t gpr_atm64; #define GPR_ATM64_MAX INT64_MAX #define GPR_ATM64_MIN INT64_MIN -#define gpr_atm_no_barrier_load(p) (__atomic_load_n((p), __ATOMIC_RELAXED)) -#define gpr_atm_no_barrier_store(p, value) \ +#define gpr_atm64_no_barrier_load(p) (__atomic_load_n((p), __ATOMIC_RELAXED)) +#define gpr_atm64_no_barrier_store(p, value) \ (__atomic_store_n((p), (int64_t)(value), __ATOMIC_RELAXED)) #ifdef __cplusplus diff --git a/src/core/lib/iomgr/timer_generic.cc b/src/core/lib/iomgr/timer_generic.cc index 8b98e015a48..211b1a1dd71 100644 --- a/src/core/lib/iomgr/timer_generic.cc +++ b/src/core/lib/iomgr/timer_generic.cc @@ -218,7 +218,7 @@ GPR_TLS_DECL(g_last_seen_min_timer); struct shared_mutables { /* The deadline of the next timer due across all timer shards */ - grpc_millis min_timer; + gpr_atm64 min_timer; /* Allow only one run_some_expired_timers at once */ gpr_spinlock checker_mu; bool initialized; @@ -418,7 +418,7 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, shard->min_deadline = deadline; note_deadline_change(shard); if (shard->shard_queue_index == 0 && deadline < old_min_deadline) { - gpr_atm_no_barrier_store(&g_shared_mutables.min_timer, deadline); + gpr_atm64_no_barrier_store(&g_shared_mutables.min_timer, deadline); grpc_kick_poller(); } } @@ -559,7 +559,8 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, grpc_error* error) { grpc_timer_check_result result = GRPC_TIMERS_NOT_CHECKED; - grpc_millis min_timer = gpr_atm_no_barrier_load(&g_shared_mutables.min_timer); + grpc_millis min_timer = + gpr_atm64_no_barrier_load(&g_shared_mutables.min_timer); #if GPR_ARCH_64 gpr_tls_set(&g_last_seen_min_timer, min_timer); #endif @@ -612,8 +613,8 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, *next = GPR_MIN(*next, g_shard_queue[0]->min_deadline); } - gpr_atm_no_barrier_store(&g_shared_mutables.min_timer, - g_shard_queue[0]->min_deadline); + gpr_atm64_no_barrier_store(&g_shared_mutables.min_timer, + g_shard_queue[0]->min_deadline); gpr_mu_unlock(&g_shared_mutables.mu); gpr_spinlock_unlock(&g_shared_mutables.checker_mu); } @@ -632,7 +633,8 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { mutable cacheline in the common case */ grpc_millis min_timer = gpr_tls_get(&g_last_seen_min_timer); #else - grpc_millis min_timer = gpr_atm_no_barrier_load(&g_shared_mutables.min_timer); + grpc_millis min_timer = + gpr_atm64_no_barrier_load(&g_shared_mutables.min_timer); #endif if (now < min_timer) { From f2f5a9a0a75b2eba8d950345aeec89b4275f7b59 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 11 Apr 2018 15:25:42 -0700 Subject: [PATCH 05/13] Keep things simple. Remove gpr_atm64 --- include/grpc/impl/codegen/atm64.h | 71 -------------------- include/grpc/impl/codegen/atm64_gcc_atomic.h | 42 ------------ include/grpc/impl/codegen/atm64_gcc_sync.h | 43 ------------ include/grpc/impl/codegen/atm64_windows.h | 50 -------------- include/grpc/support/atm64.h | 26 ------- src/core/lib/iomgr/timer_generic.cc | 46 +++++++++++-- 6 files changed, 39 insertions(+), 239 deletions(-) delete mode 100644 include/grpc/impl/codegen/atm64.h delete mode 100644 include/grpc/impl/codegen/atm64_gcc_atomic.h delete mode 100644 include/grpc/impl/codegen/atm64_gcc_sync.h delete mode 100644 include/grpc/impl/codegen/atm64_windows.h delete mode 100644 include/grpc/support/atm64.h diff --git a/include/grpc/impl/codegen/atm64.h b/include/grpc/impl/codegen/atm64.h deleted file mode 100644 index 86b0cc36272..00000000000 --- a/include/grpc/impl/codegen/atm64.h +++ /dev/null @@ -1,71 +0,0 @@ -/* - * - * Copyright 2018 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -#ifndef GRPC_IMPL_CODEGEN_ATM64_H -#define GRPC_IMPL_CODEGEN_ATM64_H - -/** This interface provides atomic operations and barriers for 64 bit integer - data types (instead of intptr_t so that this works on both 32-bit and 64-bit - systems. - - It is internal to gpr support code and should not be used outside it. - - If an operation with acquire semantics precedes another memory access by the - same thread, the operation will precede that other access as seen by other - threads. - - If an operation with release semantics follows another memory access by the - same thread, the operation will follow that other access as seen by other - threads. - - Routines with "acq" or "full" in the name have acquire semantics. Routines - with "rel" or "full" in the name have release semantics. Routines with - "no_barrier" in the name have neither acquire not release semantics. - - The routines may be implemented as macros. - - // Atomic operations act on an intergral_type gpr_atm64 that is 64 bit wide - typedef int64_t gpr_atm64; - - gpr_atm64 gpr_atm64_no_barrier_load(gpr_atm64 *p); - - // Atomically set *p = value, with release semantics. - void gpr_atm64_no_barrier_store(gpr_atm64 *p, gpr_atm64 value); -*/ - -#include - -#if defined(GPR_GCC_ATOMIC) -#include -#elif defined(GPR_GCC_SYNC) -#include -#elif defined(GPR_WINDOWS_ATOMIC) -#include -#else -#error could not determine platform for atm -#endif - -#ifdef __cplusplus -extern "C" { -#endif - -#ifdef __cplusplus -} -#endif - -#endif /* GRPC_IMPL_CODEGEN_ATM64_H */ diff --git a/include/grpc/impl/codegen/atm64_gcc_atomic.h b/include/grpc/impl/codegen/atm64_gcc_atomic.h deleted file mode 100644 index f6dc7c6b8b2..00000000000 --- a/include/grpc/impl/codegen/atm64_gcc_atomic.h +++ /dev/null @@ -1,42 +0,0 @@ -/* - * - * Copyright 2018 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -#ifndef GRPC_IMPL_CODEGEN_ATM64_GCC_ATOMIC_H -#define GRPC_IMPL_CODEGEN_ATM64_GCC_ATOMIC_H - -/* atm_platform.h for gcc and gcc-like compilers with the - __atomic_* interface. */ -#include - -#ifdef __cplusplus -extern "C" { -#endif - -typedef int64_t gpr_atm64; -#define GPR_ATM64_MAX INT64_MAX -#define GPR_ATM64_MIN INT64_MIN - -#define gpr_atm64_no_barrier_load(p) (__atomic_load_n((p), __ATOMIC_RELAXED)) -#define gpr_atm64_no_barrier_store(p, value) \ - (__atomic_store_n((p), (int64_t)(value), __ATOMIC_RELAXED)) - -#ifdef __cplusplus -} -#endif - -#endif /* GRPC_IMPL_CODEGEN_ATM64_GCC_ATOMIC_H */ diff --git a/include/grpc/impl/codegen/atm64_gcc_sync.h b/include/grpc/impl/codegen/atm64_gcc_sync.h deleted file mode 100644 index f1fcfc89e88..00000000000 --- a/include/grpc/impl/codegen/atm64_gcc_sync.h +++ /dev/null @@ -1,43 +0,0 @@ -/* - * - * Copyright 2015 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -#ifndef GRPC_IMPL_CODEGEN_ATM64_GCC_SYNC64_H -#define GRPC_IMPL_CODEGEN_ATM64_GCC_SYNC64_H - -/* variant of atm_platform.h for gcc and gcc-like compiers with __sync_* - interface */ -#include - -typedef int64_t gpr_atm64; -#define GPR_ATM64_MAX INT64_MAX -#define GPR_ATM64_MIN INT64_MIN - -#define GPR_ATM64_COMPILE_BARRIER_() __asm__ __volatile__("" : : : "memory") - -static __inline gpr_atm64 gpr_atm64_no_barrier_load(const gpr_atm64* p) { - gpr_atm64 value = *p; - GPR_ATM64_COMPILE_BARRIER_(); - return value; -} - -static __inline void gpr_atm64_no_barrier_store(gpr_atm64* p, gpr_atm64 value) { - GPR_ATM64_COMPILE_BARRIER_(); - *p = value; -} - -#endif /* GRPC_IMPL_CODEGEN_ATM64_GCC_SYNC64_H */ diff --git a/include/grpc/impl/codegen/atm64_windows.h b/include/grpc/impl/codegen/atm64_windows.h deleted file mode 100644 index df5dd6f92cb..00000000000 --- a/include/grpc/impl/codegen/atm64_windows.h +++ /dev/null @@ -1,50 +0,0 @@ -/* - * - * Copyright 2015 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -#ifndef GRPC_IMPL_CODEGEN_ATM64_WINDOWS_H -#define GRPC_IMPL_CODEGEN_ATM64_WINDOWS_H - -/** Win32 variant of atm_platform.h */ -#include - -typedef int64_t gpr_atm64; -#define GPR_ATM64_MAX INT64_MAX -#define GPR_ATM64_MIN INT64_MIN - -#define gpr_atm64_full_barrier MemoryBarrier - -static __inline gpr_atm64 gpr_atm64_acq_load(const gpr_atm64* p) { - gpr_atm64 result = *p; - gpr_atm64_full_barrier(); - return result; -} - -static __inline gpr_atm64 gpr_atm64_no_barrier_load(const gpr_atm64* p) { - return gpr_atm64_acq_load(p); -} - -static __inline void gpr_atm64_rel_store(gpr_atm64* p, gpr_atm64 value) { - gpr_atm64_full_barrier(); - *p = value; -} - -static __inline void gpr_atm64_no_barrier_store(gpr_atm64* p, gpr_atm64 value) { - gpr_atm64_rel_store(p, value); -} - -#endif /* GRPC_IMPL_CODEGEN_ATM64_WINDOWS_H */ diff --git a/include/grpc/support/atm64.h b/include/grpc/support/atm64.h deleted file mode 100644 index df5954656c0..00000000000 --- a/include/grpc/support/atm64.h +++ /dev/null @@ -1,26 +0,0 @@ -/* - * - * Copyright 2018 gRPC authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -#ifndef GRPC_SUPPORT_ATM64_H -#define GRPC_SUPPORT_ATM64_H - -#include - -#include - -#endif /* GRPC_SUPPORT_ATM64_H */ diff --git a/src/core/lib/iomgr/timer_generic.cc b/src/core/lib/iomgr/timer_generic.cc index 211b1a1dd71..9a4da596243 100644 --- a/src/core/lib/iomgr/timer_generic.cc +++ b/src/core/lib/iomgr/timer_generic.cc @@ -218,7 +218,7 @@ GPR_TLS_DECL(g_last_seen_min_timer); struct shared_mutables { /* The deadline of the next timer due across all timer shards */ - gpr_atm64 min_timer; + grpc_millis min_timer; /* Allow only one run_some_expired_timers at once */ gpr_spinlock checker_mu; bool initialized; @@ -418,7 +418,14 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, shard->min_deadline = deadline; note_deadline_change(shard); if (shard->shard_queue_index == 0 && deadline < old_min_deadline) { - gpr_atm64_no_barrier_store(&g_shared_mutables.min_timer, deadline); +#if GPR_ARCH_64 + gpr_atm_no_barrier_store(&g_shared_mutables.min_timer, deadline); +#else + // On a 32-bit system, gpr_atm_no_barrier_store does not work on 64-bit + // types (like grpc_millis). So all reads and writes to + // g_shared_mutables.min_timer varialbe under g_shared_mutables.mu + g_shared_mutables.min_timer = deadline; +#endif grpc_kick_poller(); } } @@ -559,10 +566,16 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, grpc_error* error) { grpc_timer_check_result result = GRPC_TIMERS_NOT_CHECKED; - grpc_millis min_timer = - gpr_atm64_no_barrier_load(&g_shared_mutables.min_timer); #if GPR_ARCH_64 + grpc_millis min_timer = + gpr_atm_no_barrier_load(&g_shared_mutables.min_timer); gpr_tls_set(&g_last_seen_min_timer, min_timer); +#else + // On a 32-bit system, gpr_atm_no_barrier_load does not work on 64-bit types + // (like grpc_millis). So we need to do the read under g_shared_mutables.mu + gpr_mu_lock(&g_shared_mutables.mu); + grpc_millis min_timer = g_shared_mutables.min_timer; + gpr_mu_unlock(&g_shared_mutables.mu); #endif if (now < min_timer) { if (next != nullptr) *next = GPR_MIN(*next, min_timer); @@ -613,8 +626,15 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, *next = GPR_MIN(*next, g_shard_queue[0]->min_deadline); } - gpr_atm64_no_barrier_store(&g_shared_mutables.min_timer, +#if GPR_ARCH_64 + gpr_atm_no_barrier_store(&g_shared_mutables.min_timer, g_shard_queue[0]->min_deadline); +#else + // On a 32-bit system, gpr_atm_no_barrier_store does not work on 64-bit + // types (like grpc_millis). So all reads and writes to + // g_shared_mutables.min_timer are done under g_shared_mutables.mu + g_shared_mutables.min_timer = g_shard_queue[0]->min_deadline; +#endif gpr_mu_unlock(&g_shared_mutables.mu); gpr_spinlock_unlock(&g_shared_mutables.checker_mu); } @@ -633,8 +653,14 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { mutable cacheline in the common case */ grpc_millis min_timer = gpr_tls_get(&g_last_seen_min_timer); #else - grpc_millis min_timer = - gpr_atm64_no_barrier_load(&g_shared_mutables.min_timer); + // On a 32-bit system, we do not have thread local support for 64-bit types. + // Directly read the valye from g_shared_mutables.min_timer + // Also, note that on 32-bit systems, gpr_atm_no_barrier_store does not work + // on 64-bit types like grpc_millis. So all reads and writes to + // g_shared_mutables.min_timer are done under g_shared_mutables.mu + gpr_mu_lock(&g_shared_mutables.mu); + grpc_millis min_timer = g_shared_mutables.min_timer; + gpr_mu_unlock(&g_shared_mutables.mu); #endif if (now < min_timer) { @@ -661,11 +687,17 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { } else { gpr_asprintf(&next_str, "%" PRId64, *next); } +#if GPR_ARCH_64 gpr_log(GPR_DEBUG, "TIMER CHECK BEGIN: now=%" PRId64 " next=%s tls_min=%" PRId64 " glob_min=%" PRId64, now, next_str, min_timer, gpr_atm_no_barrier_load(&g_shared_mutables.min_timer)); +#else + gpr_log(GPR_DEBUG, + "TIMER CHECK BEGIN: now=%" PRId64 " next=%s min=%" PRId64, now, + next_str, min_timer); +#endif gpr_free(next_str); } // actual code From a8c452df89987be795b1f2bf3425a4824bb16bab Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 11 Apr 2018 15:30:25 -0700 Subject: [PATCH 06/13] Remove references to gpr_atm64 --- src/core/lib/iomgr/exec_ctx.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/core/lib/iomgr/exec_ctx.h b/src/core/lib/iomgr/exec_ctx.h index 47d257d7050..4cc5586aa48 100644 --- a/src/core/lib/iomgr/exec_ctx.h +++ b/src/core/lib/iomgr/exec_ctx.h @@ -22,17 +22,16 @@ #include #include -#include #include #include #include "src/core/lib/gpr/tls.h" #include "src/core/lib/iomgr/closure.h" -typedef gpr_atm64 grpc_millis; +typedef int64_t grpc_millis; -#define GRPC_MILLIS_INF_FUTURE GPR_ATM64_MAX -#define GRPC_MILLIS_INF_PAST GPR_ATM64_MIN +#define GRPC_MILLIS_INF_FUTURE INT64_MAX +#define GRPC_MILLIS_INF_PAST INT64_MIN /** A workqueue represents a list of work to be executed asynchronously. Forward declared here to avoid a circular dependency with workqueue.h. */ From 1e1f5dcc5d56fcff7871e27d6de64e1c24123979 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 11 Apr 2018 15:41:05 -0700 Subject: [PATCH 07/13] Modify comments --- src/core/lib/iomgr/timer_generic.cc | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/core/lib/iomgr/timer_generic.cc b/src/core/lib/iomgr/timer_generic.cc index e8f14dd2e56..45259840544 100644 --- a/src/core/lib/iomgr/timer_generic.cc +++ b/src/core/lib/iomgr/timer_generic.cc @@ -431,7 +431,7 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, #if GPR_ARCH_64 gpr_atm_no_barrier_store(&g_shared_mutables.min_timer, deadline); #else - // On a 32-bit system, gpr_atm_no_barrier_store does not work on 64-bit + // On 32-bit systems, gpr_atm_no_barrier_store does not work on 64-bit // types (like grpc_millis). So all reads and writes to // g_shared_mutables.min_timer varialbe under g_shared_mutables.mu g_shared_mutables.min_timer = deadline; @@ -577,12 +577,12 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, grpc_timer_check_result result = GRPC_TIMERS_NOT_CHECKED; #if GPR_ARCH_64 - grpc_millis min_timer = - gpr_atm_no_barrier_load(&g_shared_mutables.min_timer); + grpc_millis min_timer = gpr_atm_no_barrier_load(&g_shared_mutables.min_timer); gpr_tls_set(&g_last_seen_min_timer, min_timer); #else - // On a 32-bit system, gpr_atm_no_barrier_load does not work on 64-bit types - // (like grpc_millis). So we need to do the read under g_shared_mutables.mu + // On 32-bit systems, gpr_atm_no_barrier_load does not work on 64-bit types + // (like grpc_millis). So all reads and writes to g_shared_mutables.min_timer + // are done under g_shared_mutables.mu gpr_mu_lock(&g_shared_mutables.mu); grpc_millis min_timer = g_shared_mutables.min_timer; gpr_mu_unlock(&g_shared_mutables.mu); @@ -638,9 +638,9 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, #if GPR_ARCH_64 gpr_atm_no_barrier_store(&g_shared_mutables.min_timer, - g_shard_queue[0]->min_deadline); + g_shard_queue[0]->min_deadline); #else - // On a 32-bit system, gpr_atm_no_barrier_store does not work on 64-bit + // On 32-bit systems, gpr_atm_no_barrier_store does not work on 64-bit // types (like grpc_millis). So all reads and writes to // g_shared_mutables.min_timer are done under g_shared_mutables.mu g_shared_mutables.min_timer = g_shard_queue[0]->min_deadline; @@ -663,10 +663,10 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { mutable cacheline in the common case */ grpc_millis min_timer = gpr_tls_get(&g_last_seen_min_timer); #else - // On a 32-bit system, we do not have thread local support for 64-bit types. - // Directly read the valye from g_shared_mutables.min_timer + // On 32-bit systems, we currently do not have thread local support for 64-bit + // types. In this case, directly read from g_shared_mutables.min_timer. // Also, note that on 32-bit systems, gpr_atm_no_barrier_store does not work - // on 64-bit types like grpc_millis. So all reads and writes to + // on 64-bit types (like grpc_millis). So all reads and writes to // g_shared_mutables.min_timer are done under g_shared_mutables.mu gpr_mu_lock(&g_shared_mutables.mu); grpc_millis min_timer = g_shared_mutables.min_timer; From 33643ef34e2d47dda9b460c30fe80634d18fff1a Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 11 Apr 2018 17:00:58 -0700 Subject: [PATCH 08/13] Fix type coversion errors --- src/core/lib/iomgr/ev_epollex_linux.cc | 2 +- src/core/lib/iomgr/ev_posix.cc | 4 ++-- src/core/lib/iomgr/timer_manager.cc | 3 +-- test/core/iomgr/timer_heap_test.cc | 2 +- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/core/lib/iomgr/ev_epollex_linux.cc b/src/core/lib/iomgr/ev_epollex_linux.cc index 44d8cf2b1e7..377e2866bce 100644 --- a/src/core/lib/iomgr/ev_epollex_linux.cc +++ b/src/core/lib/iomgr/ev_epollex_linux.cc @@ -1010,7 +1010,7 @@ static grpc_error* pollset_work(grpc_pollset* pollset, #endif if (grpc_polling_trace.enabled()) { gpr_log(GPR_DEBUG, - "PS:%p work hdl=%p worker=%p now=%" PRIdPTR " deadline=%" PRIdPTR + "PS:%p work hdl=%p worker=%p now=%" PRId64 " deadline=%" PRId64 " kwp=%d pollable=%p", pollset, worker_hdl, WORKER_PTR, grpc_core::ExecCtx::Get()->Now(), deadline, pollset->kicked_without_poller, pollset->active_pollable); diff --git a/src/core/lib/iomgr/ev_posix.cc b/src/core/lib/iomgr/ev_posix.cc index 8b800702653..76545c442de 100644 --- a/src/core/lib/iomgr/ev_posix.cc +++ b/src/core/lib/iomgr/ev_posix.cc @@ -244,10 +244,10 @@ static void pollset_destroy(grpc_pollset* pollset) { static grpc_error* pollset_work(grpc_pollset* pollset, grpc_pollset_worker** worker, grpc_millis deadline) { - GRPC_POLLING_API_TRACE("pollset_work(%p, %" PRIdPTR ") begin", pollset, + GRPC_POLLING_API_TRACE("pollset_work(%p, %" PRId64 ") begin", pollset, deadline); grpc_error* err = g_event_engine->pollset_work(pollset, worker, deadline); - GRPC_POLLING_API_TRACE("pollset_work(%p, %" PRIdPTR ") end", pollset, + GRPC_POLLING_API_TRACE("pollset_work(%p, %" PRId64 ") end", pollset, deadline); return err; } diff --git a/src/core/lib/iomgr/timer_manager.cc b/src/core/lib/iomgr/timer_manager.cc index 94f288af276..a14fa8d901e 100644 --- a/src/core/lib/iomgr/timer_manager.cc +++ b/src/core/lib/iomgr/timer_manager.cc @@ -172,8 +172,7 @@ static bool wait_until(grpc_millis next) { if (grpc_timer_check_trace.enabled()) { grpc_millis wait_time = next - grpc_core::ExecCtx::Get()->Now(); - gpr_log(GPR_DEBUG, "sleep for a %" PRIdPTR " milliseconds", - wait_time); + gpr_log(GPR_DEBUG, "sleep for a %" PRId64 " milliseconds", wait_time); } } else { // g_timed_waiter == true && next >= g_timed_waiter_deadline next = GRPC_MILLIS_INF_FUTURE; diff --git a/test/core/iomgr/timer_heap_test.cc b/test/core/iomgr/timer_heap_test.cc index ebe5e32f3a5..ebe5e6610d1 100644 --- a/test/core/iomgr/timer_heap_test.cc +++ b/test/core/iomgr/timer_heap_test.cc @@ -204,7 +204,7 @@ static void test2(void) { } if (num_inserted) { - gpr_atm* min_deadline = nullptr; + grpc_millis* min_deadline = nullptr; for (size_t i = 0; i < elems_size; i++) { if (elems[i].inserted) { if (min_deadline == nullptr) { From 1dd12c084a8c841dc4314a98ecf70f53bbf322a4 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 11 Apr 2018 18:05:48 -0700 Subject: [PATCH 09/13] Fix some more formatting issues --- src/core/ext/filters/client_channel/client_channel.cc | 2 +- .../ext/filters/client_channel/lb_policy/grpclb/grpclb.cc | 7 +++---- .../resolver/dns/c_ares/dns_resolver_ares.cc | 6 +++--- .../client_channel/resolver/dns/native/dns_resolver.cc | 6 +++--- src/core/ext/filters/client_channel/subchannel.cc | 2 +- src/core/lib/iomgr/pollset_custom.cc | 2 +- src/core/lib/transport/transport_op_string.cc | 2 +- .../client_channel/resolvers/dns_resolver_cooldown_test.cc | 2 +- test/core/iomgr/resolve_address_posix_test.cc | 2 +- test/core/iomgr/resolve_address_test.cc | 2 +- test/core/transport/timeout_encoding_test.cc | 2 +- test/cpp/end2end/client_lb_end2end_test.cc | 4 ++-- 12 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index a10bfea8b19..c51fb97497e 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -1399,7 +1399,7 @@ static void do_retry(grpc_call_element* elem, } if (grpc_client_channel_trace.enabled()) { gpr_log(GPR_DEBUG, - "chand=%p calld=%p: retrying failed call in %" PRIuPTR " ms", chand, + "chand=%p calld=%p: retrying failed call in %" PRId64 " ms", chand, calld, next_attempt_time - grpc_core::ExecCtx::Get()->Now()); } // Schedule retry after computed delay. diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 0b2a30818e2..79deb0ab833 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -774,7 +774,7 @@ void GrpcLb::BalancerCallState::OnBalancerMessageReceivedLocked( if (grpc_lb_glb_trace.enabled()) { gpr_log(GPR_INFO, "[grpclb %p] Received initial LB response message; " - "client load reporting interval = %" PRIdPTR " milliseconds", + "client load reporting interval = %" PRId64 " milliseconds", grpclb_policy, lb_calld->client_stats_report_interval_); } } else if (grpc_lb_glb_trace.enabled()) { @@ -1412,9 +1412,8 @@ void GrpcLb::StartBalancerCallRetryTimerLocked() { gpr_log(GPR_DEBUG, "[grpclb %p] Connection to LB server lost...", this); grpc_millis timeout = next_try - ExecCtx::Get()->Now(); if (timeout > 0) { - gpr_log(GPR_DEBUG, - "[grpclb %p] ... retry_timer_active in %" PRIuPTR "ms.", this, - timeout); + gpr_log(GPR_DEBUG, "[grpclb %p] ... retry_timer_active in %" PRId64 "ms.", + this, timeout); } else { gpr_log(GPR_DEBUG, "[grpclb %p] ... retry_timer_active immediately.", this); diff --git a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc index aca7307a2fe..9bf995fd56b 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/c_ares/dns_resolver_ares.cc @@ -345,7 +345,7 @@ void AresDnsResolver::OnResolvedLocked(void* arg, grpc_error* error) { RefCountedPtr self = r->Ref(DEBUG_LOCATION, "retry-timer"); self.release(); if (timeout > 0) { - gpr_log(GPR_DEBUG, "retrying in %" PRIdPTR " milliseconds", timeout); + gpr_log(GPR_DEBUG, "retrying in %" PRId64 " milliseconds", timeout); } else { gpr_log(GPR_DEBUG, "retrying immediately"); } @@ -371,8 +371,8 @@ void AresDnsResolver::MaybeStartResolvingLocked() { const grpc_millis last_resolution_ago = grpc_core::ExecCtx::Get()->Now() - last_resolution_timestamp_; gpr_log(GPR_DEBUG, - "In cooldown from last resolution (from %" PRIdPTR - " ms ago). Will resolve again in %" PRIdPTR " ms", + "In cooldown from last resolution (from %" PRId64 + " ms ago). Will resolve again in %" PRId64 " ms", last_resolution_ago, ms_until_next_resolution); if (!have_next_resolution_timer_) { have_next_resolution_timer_ = true; diff --git a/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc b/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc index fbab136421f..78ccf7907e9 100644 --- a/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc +++ b/src/core/ext/filters/client_channel/resolver/dns/native/dns_resolver.cc @@ -218,7 +218,7 @@ void NativeDnsResolver::OnResolvedLocked(void* arg, grpc_error* error) { r->Ref(DEBUG_LOCATION, "next_resolution_timer"); self.release(); if (timeout > 0) { - gpr_log(GPR_DEBUG, "retrying in %" PRIdPTR " milliseconds", timeout); + gpr_log(GPR_DEBUG, "retrying in %" PRId64 " milliseconds", timeout); } else { gpr_log(GPR_DEBUG, "retrying immediately"); } @@ -245,8 +245,8 @@ void NativeDnsResolver::MaybeStartResolvingLocked() { const grpc_millis last_resolution_ago = grpc_core::ExecCtx::Get()->Now() - last_resolution_timestamp_; gpr_log(GPR_DEBUG, - "In cooldown from last resolution (from %" PRIdPTR - " ms ago). Will resolve again in %" PRIdPTR " ms", + "In cooldown from last resolution (from %" PRId64 + " ms ago). Will resolve again in %" PRId64 " ms", last_resolution_ago, ms_until_next_resolution); if (!have_next_resolution_timer_) { have_next_resolution_timer_ = true; diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index d7815fb7e14..148ebbe1019 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -467,7 +467,7 @@ static void maybe_start_connecting_locked(grpc_subchannel* c) { if (time_til_next <= 0) { gpr_log(GPR_INFO, "Subchannel %p: Retry immediately", c); } else { - gpr_log(GPR_INFO, "Subchannel %p: Retry in %" PRIdPTR " milliseconds", c, + gpr_log(GPR_INFO, "Subchannel %p: Retry in %" PRId64 " milliseconds", c, time_til_next); } GRPC_CLOSURE_INIT(&c->on_alarm, on_alarm, c, grpc_schedule_on_exec_ctx); diff --git a/src/core/lib/iomgr/pollset_custom.cc b/src/core/lib/iomgr/pollset_custom.cc index 04bd1040552..783f46c6d60 100644 --- a/src/core/lib/iomgr/pollset_custom.cc +++ b/src/core/lib/iomgr/pollset_custom.cc @@ -71,7 +71,7 @@ static grpc_error* pollset_work(grpc_pollset* pollset, grpc_millis now = grpc_core::ExecCtx::Get()->Now(); size_t timeout = 0; if (deadline > now) { - timeout = deadline - now; + timeout = (size_t)(deadline - now); } // We yield here because the poll() call might yield // control back to the application diff --git a/src/core/lib/transport/transport_op_string.cc b/src/core/lib/transport/transport_op_string.cc index 99af7c1931e..25ab492f3a4 100644 --- a/src/core/lib/transport/transport_op_string.cc +++ b/src/core/lib/transport/transport_op_string.cc @@ -52,7 +52,7 @@ static void put_metadata_list(gpr_strvec* b, grpc_metadata_batch md) { } if (md.deadline != GRPC_MILLIS_INF_FUTURE) { char* tmp; - gpr_asprintf(&tmp, " deadline=%" PRIdPTR, md.deadline); + gpr_asprintf(&tmp, " deadline=%" PRId64, md.deadline); gpr_strvec_add(b, tmp); } } diff --git a/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc b/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc index 01c61a9f186..4fd7aedc6c3 100644 --- a/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc +++ b/test/core/client_channel/resolvers/dns_resolver_cooldown_test.cc @@ -128,7 +128,7 @@ static void poll_pollset_until_request_done(iomgr_args* args) { break; } grpc_millis time_left = deadline - grpc_core::ExecCtx::Get()->Now(); - gpr_log(GPR_DEBUG, "done=%d, time_left=%" PRIdPTR, done, time_left); + gpr_log(GPR_DEBUG, "done=%d, time_left=%" PRId64, done, time_left); GPR_ASSERT(time_left >= 0); grpc_pollset_worker* worker = nullptr; gpr_mu_lock(args->mu); diff --git a/test/core/iomgr/resolve_address_posix_test.cc b/test/core/iomgr/resolve_address_posix_test.cc index 79b2b50e702..e495e4c8770 100644 --- a/test/core/iomgr/resolve_address_posix_test.cc +++ b/test/core/iomgr/resolve_address_posix_test.cc @@ -91,7 +91,7 @@ static void actually_poll(void* argsp) { break; } grpc_millis time_left = deadline - grpc_core::ExecCtx::Get()->Now(); - gpr_log(GPR_DEBUG, "done=%d, time_left=%" PRIdPTR, done, time_left); + gpr_log(GPR_DEBUG, "done=%d, time_left=%" PRId64, done, time_left); GPR_ASSERT(time_left >= 0); grpc_pollset_worker* worker = nullptr; gpr_mu_lock(args->mu); diff --git a/test/core/iomgr/resolve_address_test.cc b/test/core/iomgr/resolve_address_test.cc index a0dc484f3e4..2fb831a6a46 100644 --- a/test/core/iomgr/resolve_address_test.cc +++ b/test/core/iomgr/resolve_address_test.cc @@ -82,7 +82,7 @@ static void poll_pollset_until_request_done(args_struct* args) { break; } grpc_millis time_left = deadline - grpc_core::ExecCtx::Get()->Now(); - gpr_log(GPR_DEBUG, "done=%d, time_left=%" PRIdPTR, done, time_left); + gpr_log(GPR_DEBUG, "done=%d, time_left=%" PRId64, done, time_left); GPR_ASSERT(time_left >= 0); grpc_pollset_worker* worker = nullptr; gpr_mu_lock(args->mu); diff --git a/test/core/transport/timeout_encoding_test.cc b/test/core/transport/timeout_encoding_test.cc index 2367accc2e7..b7044b5b410 100644 --- a/test/core/transport/timeout_encoding_test.cc +++ b/test/core/transport/timeout_encoding_test.cc @@ -71,7 +71,7 @@ static void assert_decodes_as(const char* buffer, grpc_millis expected) { GPR_ASSERT(1 == grpc_http2_decode_timeout( grpc_slice_from_static_string(buffer), &got)); if (got != expected) { - gpr_log(GPR_ERROR, "got:'%" PRIdPTR "' != expected:'%" PRIdPTR "'", got, + gpr_log(GPR_ERROR, "got:'%" PRId64 "' != expected:'%" PRId64 "'", got, expected); abort(); } diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index a39e443cf0b..c2f3281fbe9 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -362,7 +362,7 @@ TEST_F(ClientLbEnd2endTest, PickFirstBackOffInitialReconnect) { grpc_timeout_milliseconds_to_deadline(kInitialBackOffMs * 2))); const gpr_timespec t1 = gpr_now(GPR_CLOCK_MONOTONIC); const grpc_millis waited_ms = gpr_time_to_millis(gpr_time_sub(t1, t0)); - gpr_log(GPR_DEBUG, "Waited %ld milliseconds", waited_ms); + gpr_log(GPR_DEBUG, "Waited %" PRId64 " milliseconds", waited_ms); // We should have waited at least kInitialBackOffMs. We substract one to // account for test and precision accuracy drift. EXPECT_GE(waited_ms, kInitialBackOffMs - 1); @@ -391,7 +391,7 @@ TEST_F(ClientLbEnd2endTest, PickFirstBackOffMinReconnect) { grpc_timeout_milliseconds_to_deadline(kMinReconnectBackOffMs * 2)); const gpr_timespec t1 = gpr_now(GPR_CLOCK_MONOTONIC); const grpc_millis waited_ms = gpr_time_to_millis(gpr_time_sub(t1, t0)); - gpr_log(GPR_DEBUG, "Waited %ld ms", waited_ms); + gpr_log(GPR_DEBUG, "Waited %" PRId64 " ms", waited_ms); // We should have waited at least kMinReconnectBackOffMs. We substract one to // account for test and precision accuracy drift. EXPECT_GE(waited_ms, kMinReconnectBackOffMs - 1); From b5f1941bca69544e7ea4b6fe7b110834d655a0b8 Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Wed, 18 Apr 2018 15:45:26 -0700 Subject: [PATCH 10/13] Avoid C-style casts --- src/core/lib/iomgr/pollset_custom.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/lib/iomgr/pollset_custom.cc b/src/core/lib/iomgr/pollset_custom.cc index 783f46c6d60..601adc6d6a8 100644 --- a/src/core/lib/iomgr/pollset_custom.cc +++ b/src/core/lib/iomgr/pollset_custom.cc @@ -71,7 +71,7 @@ static grpc_error* pollset_work(grpc_pollset* pollset, grpc_millis now = grpc_core::ExecCtx::Get()->Now(); size_t timeout = 0; if (deadline > now) { - timeout = (size_t)(deadline - now); + timeout = static_cast(deadline - now); } // We yield here because the poll() call might yield // control back to the application From dc01cb64c25abc14f6d0ab24db1afd019a37d7ee Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Tue, 8 May 2018 10:07:30 -0700 Subject: [PATCH 11/13] Correct typecasts --- src/core/lib/iomgr/pollset_custom.cc | 6 +++--- src/core/lib/iomgr/timer_generic.cc | 14 +++++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/core/lib/iomgr/pollset_custom.cc b/src/core/lib/iomgr/pollset_custom.cc index 601adc6d6a8..70e8a4596fa 100644 --- a/src/core/lib/iomgr/pollset_custom.cc +++ b/src/core/lib/iomgr/pollset_custom.cc @@ -69,15 +69,15 @@ static grpc_error* pollset_work(grpc_pollset* pollset, GRPC_CUSTOM_IOMGR_ASSERT_SAME_THREAD(); gpr_mu_unlock(&pollset->mu); grpc_millis now = grpc_core::ExecCtx::Get()->Now(); - size_t timeout = 0; + grpc_millis timeout = 0; if (deadline > now) { - timeout = static_cast(deadline - now); + timeout = deadline - now; } // We yield here because the poll() call might yield // control back to the application grpc_core::ExecCtx* curr = grpc_core::ExecCtx::Get(); grpc_core::ExecCtx::Set(nullptr); - poller_vtable->poll(timeout); + poller_vtable->poll(static_cast(timeout)); grpc_core::ExecCtx::Set(curr); grpc_core::ExecCtx::Get()->InvalidateNow(); if (grpc_core::ExecCtx::Get()->HasWork()) { diff --git a/src/core/lib/iomgr/timer_generic.cc b/src/core/lib/iomgr/timer_generic.cc index 45259840544..d3245dee4c5 100644 --- a/src/core/lib/iomgr/timer_generic.cc +++ b/src/core/lib/iomgr/timer_generic.cc @@ -429,7 +429,8 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, note_deadline_change(shard); if (shard->shard_queue_index == 0 && deadline < old_min_deadline) { #if GPR_ARCH_64 - gpr_atm_no_barrier_store(&g_shared_mutables.min_timer, deadline); + gpr_atm_no_barrier_store( + static_cast(&g_shared_mutables.min_timer), deadline); #else // On 32-bit systems, gpr_atm_no_barrier_store does not work on 64-bit // types (like grpc_millis). So all reads and writes to @@ -577,7 +578,8 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, grpc_timer_check_result result = GRPC_TIMERS_NOT_CHECKED; #if GPR_ARCH_64 - grpc_millis min_timer = gpr_atm_no_barrier_load(&g_shared_mutables.min_timer); + grpc_millis min_timer = static_cast(gpr_atm_no_barrier_load( + static_cast(&g_shared_mutables.min_timer))); gpr_tls_set(&g_last_seen_min_timer, min_timer); #else // On 32-bit systems, gpr_atm_no_barrier_load does not work on 64-bit types @@ -637,8 +639,9 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, } #if GPR_ARCH_64 - gpr_atm_no_barrier_store(&g_shared_mutables.min_timer, - g_shard_queue[0]->min_deadline); + gpr_atm_no_barrier_store( + static_cast(&g_shared_mutables.min_timer), + g_shard_queue[0]->min_deadline); #else // On 32-bit systems, gpr_atm_no_barrier_store does not work on 64-bit // types (like grpc_millis). So all reads and writes to @@ -702,7 +705,8 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { "TIMER CHECK BEGIN: now=%" PRId64 " next=%s tls_min=%" PRId64 " glob_min=%" PRId64, now, next_str, min_timer, - gpr_atm_no_barrier_load(&g_shared_mutables.min_timer)); + gpr_atm_no_barrier_load( + static_cast(&g_shared_mutables.min_timer))); #else gpr_log(GPR_DEBUG, "TIMER CHECK BEGIN: now=%" PRId64 " next=%s min=%" PRId64, now, From 57ed906b4f702832950c0eff0818db8bf308274b Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Fri, 11 May 2018 11:05:24 -0700 Subject: [PATCH 12/13] Fix static_cast errors --- src/core/lib/iomgr/timer_generic.cc | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/core/lib/iomgr/timer_generic.cc b/src/core/lib/iomgr/timer_generic.cc index 2bbc9d8874d..e581e113587 100644 --- a/src/core/lib/iomgr/timer_generic.cc +++ b/src/core/lib/iomgr/timer_generic.cc @@ -429,8 +429,12 @@ static void timer_init(grpc_timer* timer, grpc_millis deadline, note_deadline_change(shard); if (shard->shard_queue_index == 0 && deadline < old_min_deadline) { #if GPR_ARCH_64 - gpr_atm_no_barrier_store( - static_cast(&g_shared_mutables.min_timer), deadline); + // TODO: sreek - Using c-style cast here. static_cast<> gives an error + // (on mac platforms complaining that gpr_atm* is (long *) while + // (&g_shared_mutables.min_timer) is a (long long *). The cast should be + // safe since we know that both are pointer types and 64-bit wide. + gpr_atm_no_barrier_store((gpr_atm*)(&g_shared_mutables.min_timer), + deadline); #else // On 32-bit systems, gpr_atm_no_barrier_store does not work on 64-bit // types (like grpc_millis). So all reads and writes to @@ -578,8 +582,12 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, grpc_timer_check_result result = GRPC_TIMERS_NOT_CHECKED; #if GPR_ARCH_64 - grpc_millis min_timer = static_cast(gpr_atm_no_barrier_load( - static_cast(&g_shared_mutables.min_timer))); + // TODO: sreek - Using c-style cast here. static_cast<> gives an error (on + // mac platforms complaining that gpr_atm* is (long *) while + // (&g_shared_mutables.min_timer) is a (long long *). The cast should be + // safe since we know that both are pointer types and 64-bit wide + grpc_millis min_timer = static_cast( + gpr_atm_no_barrier_load((gpr_atm*)(&g_shared_mutables.min_timer))); gpr_tls_set(&g_last_seen_min_timer, min_timer); #else // On 32-bit systems, gpr_atm_no_barrier_load does not work on 64-bit types @@ -639,9 +647,12 @@ static grpc_timer_check_result run_some_expired_timers(grpc_millis now, } #if GPR_ARCH_64 - gpr_atm_no_barrier_store( - static_cast(&g_shared_mutables.min_timer), - g_shard_queue[0]->min_deadline); + // TODO: sreek - Using c-style cast here. static_cast<> gives an error (on + // mac platforms complaining that gpr_atm* is (long *) while + // (&g_shared_mutables.min_timer) is a (long long *). The cast should be + // safe since we know that both are pointer types and 64-bit wide + gpr_atm_no_barrier_store((gpr_atm*)(&g_shared_mutables.min_timer), + g_shard_queue[0]->min_deadline); #else // On 32-bit systems, gpr_atm_no_barrier_store does not work on 64-bit // types (like grpc_millis). So all reads and writes to @@ -705,8 +716,7 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { "TIMER CHECK BEGIN: now=%" PRId64 " next=%s tls_min=%" PRId64 " glob_min=%" PRId64, now, next_str, min_timer, - gpr_atm_no_barrier_load( - static_cast(&g_shared_mutables.min_timer))); + gpr_atm_no_barrier_load((gpr_atm*)(&g_shared_mutables.min_timer))); #else gpr_log(GPR_INFO, "TIMER CHECK BEGIN: now=%" PRId64 " next=%s min=%" PRId64, now, next_str, min_timer); From a2171d49bfacc2a9f364a17ee50c6d89767cdb6c Mon Sep 17 00:00:00 2001 From: Sree Kuchibhotla Date: Tue, 15 May 2018 15:20:46 -0700 Subject: [PATCH 13/13] Fix format specifier --- src/core/lib/iomgr/timer_generic.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/lib/iomgr/timer_generic.cc b/src/core/lib/iomgr/timer_generic.cc index e581e113587..4294162af7c 100644 --- a/src/core/lib/iomgr/timer_generic.cc +++ b/src/core/lib/iomgr/timer_generic.cc @@ -714,7 +714,7 @@ static grpc_timer_check_result timer_check(grpc_millis* next) { #if GPR_ARCH_64 gpr_log(GPR_INFO, "TIMER CHECK BEGIN: now=%" PRId64 " next=%s tls_min=%" PRId64 - " glob_min=%" PRId64, + " glob_min=%" PRIdPTR, now, next_str, min_timer, gpr_atm_no_barrier_load((gpr_atm*)(&g_shared_mutables.min_timer))); #else