Skip to content

Fix ParkingLot::signal does not modify _pending_signal when there is no waiter#2916

Merged
wwbmmm merged 1 commit intoapache:masterfrom
chenBright:fix_pl
Mar 15, 2025
Merged

Fix ParkingLot::signal does not modify _pending_signal when there is no waiter#2916
wwbmmm merged 1 commit intoapache:masterfrom
chenBright:fix_pl

Conversation

@chenBright
Copy link
Copy Markdown
Contributor

@chenBright chenBright commented Mar 14, 2025

What problem does this PR solve?

Issue Number:

Problem Summary:

#2907 合入后,CI经常超时,都是发生在EventDispatcherTest.dispatch_tasksKeyTest.creating_key_in_parallel

原因:signal早于waitsignal没改_pending_signal,信号丢失导致后续的wait不会立即返回。

What is changed and the side effects?

Changed:

无论有无waiter,signal都要改_pending_signal

Side effects:

  • Performance effects(性能影响):

  • Breaking backward compatibility(向后兼容性):


Check List:

  • Please make sure your changes are compilable(请确保你的更改可以通过编译).
  • When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
  • Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

@chenBright chenBright added the bug the code does not work as expected label Mar 14, 2025
@wwbmmm wwbmmm merged commit df0fbdc into apache:master Mar 15, 2025
20 checks passed
@chenBright chenBright deleted the fix_pl branch March 15, 2025 13:12
@zhoukangsheng
Copy link
Copy Markdown
Contributor

我参照2907的修改进行了线上小流量验证,发现tp99上涨了约2-3ms,原先tp99大概是450ms,是否也是这个原因

@yanglimingcn
Copy link
Copy Markdown
Contributor

我参照2907的修改进行了线上小流量验证,发现tp99上涨了约2-3ms,原先tp99大概是450ms,是否也是这个原因

这个不修改会产生bug,这个应该是必须要修改的,我这边遇到问题了。

@JimChengLin
Copy link
Copy Markdown
Contributor

确实会有问题,我之前线上没注意到这个问题。

如果你的任务来自于 worker,parking lot 的 waiter 有可能唤醒没那么频繁,但不会信号丢失。因为 signal 的 worker 本身最终会把 task 消费掉。这个 bug/feature 会进一步减少 futex wake,降低 CPU 占用。

如果任务来自于 non-worker,除了唤醒不那么频繁,叠加这种极端情况,即所有的 worker 都在准备 futex wait,同时有一个 non-worker 投递了一个 task,这样就会产生信号丢失,task 就永远不会被消费了。这种场景发生概率应该很低,大家可以 check 一下是不是遇到这种了。

@yanglimingcn
Copy link
Copy Markdown
Contributor

确实会有问题,我之前线上没注意到这个问题。

如果你的任务来自于 worker,parking lot 的 waiter 有可能唤醒没那么频繁,但不会信号丢失。因为 signal 的 worker 本身最终会把 task 消费掉。这个 bug/feature 会进一步减少 futex wake,降低 CPU 占用。

如果任务来自于 non-worker,除了唤醒不那么频繁,叠加这种极端情况,即所有的 worker 都在准备 futex wait,同时有一个 non-worker 投递了一个 task,这样就会产生信号丢失,task 就永远不会被消费了。这种场景发生概率应该很低,大家可以 check 一下是不是遇到这种了。

现在signal里面的由最开始的futex_wake_private变为原子变量_waiter_num的检查,我不太清楚收益有多大,我这边没测试过。

@JimChengLin
Copy link
Copy Markdown
Contributor

yanglimingcn

有巨大的差别,syscall 的成本是 us 级别的,atomic check 是 ns 级别的。

@zhoukangsheng
Copy link
Copy Markdown
Contributor

我参照2907的修改进行了线上小流量验证,发现tp99上涨了约2-3ms,原先tp99大概是450ms,是否也是这个原因

按照这个bug_fix的修改进行了验证,tp99确实恢复了。

@JimChengLin
Copy link
Copy Markdown
Contributor

我参照2907的修改进行了线上小流量验证,发现tp99上涨了约2-3ms,原先tp99大概是450ms,是否也是这个原因

按照这个bug_fix的修改进行了验证,tp99确实恢复了。

👍,我也去测试一下

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug the code does not work as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants