-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix for accessors lut invalidation #5132
fix for accessors lut invalidation #5132
Conversation
Ah yes, obviously, you have to make sure |
From my benchmarks of operations on LogStash::Event, the performance change with this patch is as follows:
All these are measured with an uncertainty around +/- 4% So IMO, since this actually fixes a bug, I think the PR is on the right track 👍 [edit] see more about the results in https://gist.github.com/jsvd/172c98c3d6bc6cc08aca6501d60cc7a9 |
@jsvd thanks for measuring this! |
9e27772
to
30a38c9
Compare
Note that @jsvd @guyboertje @talevy please review? |
The build failure is not related to this PR
|
@colinsurprenant I will look into this failling test. first time I see it. |
describe "Accessors invalidation" do | ||
let(:event) { LogStash::Event.new({ "message" => "foo" }) } | ||
|
||
it "should perperly invalidate accessors" 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.
properly
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.
👍
LGTM |
@@ -67,7 +67,7 @@ private Object findTarget(FieldReference field) { | |||
target = this.data; | |||
for (String key : field.getPath()) { | |||
target = fetch(target, key); | |||
if (target == null) { | |||
if (! targetIsCollection(target)) { |
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.
this method only checks that the argument is a collection, I suggest renaming it as isCollection
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.
I was totally expecting this comment :D
2 minor comments, otherwise tests went well, LGTM |
@@ -134,7 +138,7 @@ private Object fetch(Object target, String key) { | |||
} else if (target == null) { | |||
return null; | |||
} { |
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.
woa, this is a weird syntax quirk
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.
missing else
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.
yup... weird omission, it was still "correct" but ... anyhow. fixing :D
LGTM |
148b028
to
5ec0776
Compare
lut invalidation spec fix lut invalidation extra comment fix lut invalidation, invalid target path, with tests post review fixes
5ec0776
to
d89fd80
Compare
merged in master, 5.0, 2.x. will wait after 2.3.2 release (today) to merge fix in 2.3, we prefer to get a bit more mileage with this fix before it makes it into a release. |
This is a fix for the Accessors fieldref cache invalidation in the Ruby Event only. Before considering merging this we should assess the performance impact and see if using a hierarchical cache would provide better performance. Depending on the direction we choose for the fix, the same idea will have to be applied to the Java Event.
A stale cache spec has been added.
Fixes #5127
[edit 04/20]
Per comments below, we agreed that this fix is acceptable performance-wise and the same fix has been applied to the Java Event. Doing so uncovered another bug that resulted in a
expecting List or Map
ClassCastException, also fixed and related test added.