-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Minor performance improvements #4302
Conversation
Signed-off-by: Šimon Lukašík <[email protected]>
Signed-off-by: Šimon Lukašík <[email protected]>
Signed-off-by: Šimon Lukašík <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I'm worried about one point.
I would like to hear other maintainers' opinions on the following points.
I am seeing segfault in the CI for Ruby head, although, I don't think it is related to this PR. |
Yes, currently, the CI doesn't work on Ruby head. I am concerned that the CI on macOS is unstable. |
I have reviewed several failed macos jobs in CI and here I am presenting a summary of what I saw. I have seen 6 different errors reported on macos. None of them is immediately deterministic. no nodes available (4 occurences)
unwanched files must be removed (5 occurences)
IOError: closed stream
recv not defined
supervisor signal not handled
can execute external command many times, which finishes immediately
|
Thanks for checking CI.
|
lib/fluent/test/driver/base.rb
Outdated
@@ -87,7 +87,7 @@ def run(timeout: nil, start: true, shutdown: true, &block) | |||
|
|||
sleep_with_watching_threads = ->(){ | |||
if @instance.respond_to?(:_threads) | |||
@instance._threads.values.each{|t| t.join(0) } | |||
@instance._threads.each_value{|t| t.join(0) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this error did not happen before.
I'm checking if this change has any impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it has the impact. I am trying to understand how this relates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that it is caused by the fact that the code
@instance._threads.each_value{|t| t.join(0) }
is accessing @_threads
instance varible from PluginHelperThread
mixin, that is otherwise guarded by @_threads_mutex
, but not in this case.
I will study a code a bit more, but in the mean time, I will remove this affected chunk from the PR.
Thank You for your help, @daipom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Looks like this error no longer occurs.
I don't understand why this change causes this error and why this error is specific to macOS...
If you find out anything, please let us know. Thanks!
In the meantime, how about adding some comments to this line? such as
# Can not use each_value here. Somehow, it makes tests on macOS unstable.
# https://github.com/fluent/fluentd/pull/4302#issuecomment-1746490368
For now, I think the current modifications are fine.
I will merge this later.
Signed-off-by: Šimon Lukašík <[email protected]>
Thanks for the improvements! |
Minor, local, and inconsequential performance improvements.