-
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
Optimize multiple filters call #1145
Optimize multiple filters call #1145
Conversation
You should also measure one filter case. |
038127e
to
6bcf841
Compare
if optimizable? | ||
optimized_filter_stream(tag, es) | ||
else | ||
$log.info "Can't optimized filters, because some plugins implement `filter_stream`" |
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.
Log messages should be for users, not only for plugin developers.
So it's better to show "filtering works with worse performance, 'xxx_filter' users #filter_stream method" or so.
Users will get known about performance, and can complaint on that plugin about implementation.
6bcf841
to
d74b3b7
Compare
@repeatedly The result of calling
The result of calling
This is
|
2cbaff5
to
3165a0d
Compare
@filters.each do |filter| | ||
if filter.has_filter_with_time | ||
begin | ||
filtered_time, filtered_record = *filter.filter_with_time(tag, filtered_time, filtered_record) |
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.
There are no need to use splat (*) to return 2 values from method
Some required test cases are missing. For example:
This pull-request includes the change how to handle/call methods of filters. So tests above are required. |
Just a thought, stackprof is better for performance tuning. |
9b46e48
to
860b7be
Compare
e46bf8a
to
da6a163
Compare
Added these tests.
|
@now = Engine.now | ||
end | ||
|
||
test 'call optimized filter when filter plugin that has #method' do |
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.
#filter
instead of #method
, right?
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.
@ganmacs please check and fix the subjects of tests newly added (especially for English syntax etc). |
60dad98
to
32c875d
Compare
end | ||
end | ||
|
||
test "don't call optimized filter whenf only one of some filters has #filter_stream method" do |
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.
"whenf"
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.
"even if just a filter of some filters implements #filter_stream
method"?
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.
end | ||
end | ||
|
||
test 'call optimized filter when filter plugin that has #filter_with_time' do |
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.
"when the filter plugin implements #filter_with_time
without #filter_stream
"
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.
32c875d
to
e1de49e
Compare
e1de49e
to
7bf115b
Compare
LGTM. Thank you! |
Because in v0.14, #filter will be optimized. So, we can remove this method for v0.14. see: fluent/fluentd#1145
Because in v0.14, #filter will be optimized. So, we can remove this method for v0.14. see: fluent/fluentd#1145
Please merge this PR after #1140 is merged, Becasue this PR depends on #1140.
If your code can be optimized, the speed of
filter_stream
will be about 1.2 times faster.This result is caused by avoiding to call a lot of
Fluent::MultiEventStream#add
.the requirement of running optimization is that filter plugins that you are using don't implement
filter_stream
.the executed command is here.
The measured code, as shown below. (https://github.com/fluent/fluentd/pull/1145/files#diff-c18beef6caee95367d6268a4c181f913R166)
The result o calling
optimized_filter_stream
This result of calling
@filters.reduce(es) { |acc, filter| filter.filter_stream(tag, acc) }
Environment
2.7 GHz Intel Core i5
8 GB 1867 MHz DDR3