fix walking back StartOfElapsingTime #4555
Conversation
kruall
requested review from
snaury and
dcherednik
and removed request for
dcherednik
|
⚪ |
|
⚪ |
| @@ -266,7 +268,10 @@ namespace NActors { | |||
| if (mailbox->IsEmpty()) // was not-free and become free, we must reclaim mailbox | |||
| reclaimAsFree = true; | |||
|
|
|||
| NHPTimer::STime elapsed = Ctx.AddEventProcessingStats(hpprev, hpnow, activityType, CurrentActorScheduledEventsCounter); | |||
| if (Y_LIKELY(hpprev < hpnow)) { | |||
There was a problem hiding this comment.
Я думаю эта проверка должны быть внутри AddEventProcessingStats, чтобы мы например учитывать затраченное время как 0, но при этом увеличивать ReceivedEvents и т.д.
| hpprev = TlsThreadContext->StartOfElapsingTime.exchange(hpnow, std::memory_order_acq_rel); | ||
| Ctx.AddElapsedCycles(ActorSystemIndex, hpnow - hpprev); | ||
| hpprev = TlsThreadContext->UpdateStartOfElapsingTime(hpnow); | ||
| if (Y_LIKELY(hpprev < hpnow)) { |
There was a problem hiding this comment.
Я бы предложил проверки на > 0 унести внутрь AddParkedCycles/AddElapsedCycles, чтобы не дублировать их по много раз.
| ui64 UpdateStartOfElapsingTime(ui64 newValue) { | ||
| ui64 oldValue = StartOfElapsingTime.load(std::memory_order_acquire); | ||
| for (;;) { | ||
| if (newValue <= oldValue) { |
There was a problem hiding this comment.
С одной стороны tsc счётчика должно хватать на 190 лет (на частоте 3GHz), с другой стороны я не уверен начинается ли он всегда с нуля. Может быть вычислять newValue - oldValue в виде i64 и проверять что оно больше нуля? В генерируемом коде при этом разницы почти не будет, зато на случай overflow будет соломка.
| @@ -266,7 +268,10 @@ namespace NActors { | |||
| if (mailbox->IsEmpty()) // was not-free and become free, we must reclaim mailbox | |||
| reclaimAsFree = true; | |||
|
|
|||
| NHPTimer::STime elapsed = Ctx.AddEventProcessingStats(hpprev, hpnow, activityType, CurrentActorScheduledEventsCounter); | |||
| if (Y_LIKELY(hpprev < hpnow)) { | |||
| Ctx.AddEventProcessingStats(hpprev, hpnow, activityType, CurrentActorScheduledEventsCounter); | |||
There was a problem hiding this comment.
И кстати в EventProcessingTime (гистограмму) мы должны добавлять hpnow - eventStart. Если другой поток при сборе статистики часть времени откусил, это не значит что обработка события вдруг стала быстрой.
kruall
force-pushed
the
as/fix_walking_back_StartOfElapsingTime
branch
from
d8776f2 to
af87841
Compare
|
⚪
|
|
⚪
|
Changelog entry
...
Changelog category
Additional information
...