fix for accessors lut invalidation #5132

colinsurprenant · 2016-04-18T19:12:46Z

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.

colinsurprenant · 2016-04-18T19:30:05Z

Ah yes, obviously, you have to make sure logstash-core-event is selected in the Gemfile and logstash-core.gemspec to test with the Ruby Event.

jsvd · 2016-04-19T14:51:02Z

From my benchmarks of operations on LogStash::Event, the performance change with this patch is as follows:

in a set-heavy benchmark, this patch is ~7% faster than the current implementation
in a read-heavy benchmark, this patch is ~4.8% slower than current implementation
in a mixed benchmark, this patch is ~4.5% faster than the current implementation

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

colinsurprenant · 2016-04-19T15:05:39Z

@jsvd thanks for measuring this!

colinsurprenant · 2016-04-20T21:50:07Z

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.

Note that rake test:core reports the error under both Event implementations if/when removing the fix. The Java implementation also includes specific Java unit tests.

@jsvd @guyboertje @talevy please review?

colinsurprenant · 2016-04-20T22:27:26Z

The build failure is not related to this PR

Failures:

  1) LogStash::Agent metrics after config reloading resets the metric collector
     Failure/Error: expect(snapshot.metric_store.get_with_path("/stats/events")[:stats][:events][:in].value).to eq(new_config_generator_counter)
     NoMethodError:
       undefined method `value' for nil:NilClass
     # ./logstash-core/spec/logstash/agent_spec.rb:374:in `(root)'
     # ./vendor/bundle/jruby/1.9/gems/rspec-wait-0.0.8/lib/rspec/wait.rb:46:in `(root)'
     # ./rakelib/test.rake:42:in `(root)'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/task.rb:248:in `execute'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/task.rb:243:in `execute'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/task.rb:187:in `invoke_with_call_chain'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/task.rb:180:in `invoke_with_call_chain'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/task.rb:173:in `invoke'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/application.rb:150:in `invoke_task'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/application.rb:106:in `top_level'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/application.rb:106:in `top_level'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/application.rb:115:in `run_with_threads'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/application.rb:100:in `top_level'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/application.rb:78:in `run'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/application.rb:176:in `standard_exception_handling'
     # ./vendor/bundle/jruby/1.9/gems/rake-11.1.2/lib/rake/application.rb:75:in `run'

Finished in 38.1 seconds (files took 2.1 seconds to load)
1179 examples, 1 failure

Failed examples:

rspec ./logstash-core/spec/logstash/agent_spec.rb:359 # LogStash::Agent metrics after config reloading resets the metric collector

Randomized with seed 54320

@ph @purbon ?

ph · 2016-04-21T00:20:48Z

@colinsurprenant I will look into this failling test. first time I see it.

guyboertje · 2016-04-21T14:35:49Z

logstash-core-event/spec/logstash/event_spec.rb

+  describe "Accessors invalidation" do
+    let(:event) { LogStash::Event.new({ "message" => "foo" }) }
+
+    it "should perperly invalidate accessors" do


guyboertje · 2016-04-21T14:45:46Z

LGTM

jsvd · 2016-04-21T15:47:37Z

logstash-core-event-java/src/main/java/com/logstash/Accessors.java

@@ -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)) {


this method only checks that the argument is a collection, I suggest renaming it as isCollection

I was totally expecting this comment :D

jsvd · 2016-04-21T15:51:41Z

2 minor comments, otherwise tests went well, LGTM

colinsurprenant · 2016-04-21T16:32:08Z

logstash-core-event-java/src/main/java/com/logstash/Accessors.java

@@ -134,7 +138,7 @@ private Object fetch(Object target, String key) {
        } else if (target == null) {
            return null;
        } {


woa, this is a weird syntax quirk

missing else

yup... weird omission, it was still "correct" but ... anyhow. fixing :D

colinsurprenant · 2016-04-21T17:28:28Z

ok, pushed changes. any final review @jsvd @talevy ?

jsvd · 2016-04-22T13:53:18Z

LGTM

lut invalidation spec fix lut invalidation extra comment fix lut invalidation, invalid target path, with tests post review fixes

colinsurprenant · 2016-04-25T15:24:18Z

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.

colinsurprenant added the bug label Apr 18, 2016

colinsurprenant changed the title ~~fix for accessors lut invalidation~~ [WIP] fix for accessors lut invalidation Apr 18, 2016

jsvd self-assigned this Apr 19, 2016

colinsurprenant mentioned this pull request Apr 20, 2016

New Event API proposal #5141

Closed

colinsurprenant force-pushed the fix/accessors_lut branch from 9e27772 to 30a38c9 Compare April 20, 2016 18:29

suyograo added the v5.0.0-alpha2 label Apr 20, 2016

colinsurprenant changed the title ~~[WIP] fix for accessors lut invalidation~~ fix for accessors lut invalidation Apr 20, 2016

ph mentioned this pull request Apr 21, 2016

Failling metrics test on master #5152

Closed

guyboertje reviewed Apr 21, 2016
View reviewed changes

jsvd reviewed Apr 21, 2016
View reviewed changes

colinsurprenant reviewed Apr 21, 2016
View reviewed changes

colinsurprenant force-pushed the fix/accessors_lut branch from 148b028 to 5ec0776 Compare April 25, 2016 14:54

Ruby and Java Event Accessors lut invalidation

d89fd80

lut invalidation spec fix lut invalidation extra comment fix lut invalidation, invalid target path, with tests post review fixes

colinsurprenant force-pushed the fix/accessors_lut branch from 5ec0776 to d89fd80 Compare April 25, 2016 15:02

colinsurprenant merged commit d89fd80 into elastic:master Apr 25, 2016

suyograo added the v5.0.0 label May 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for accessors lut invalidation #5132

fix for accessors lut invalidation #5132

colinsurprenant commented Apr 18, 2016 •

edited

Loading

colinsurprenant commented Apr 18, 2016 •

edited

Loading

jsvd commented Apr 19, 2016 •

edited

Loading

colinsurprenant commented Apr 19, 2016

colinsurprenant commented Apr 20, 2016 •

edited

Loading

colinsurprenant commented Apr 20, 2016

ph commented Apr 21, 2016

guyboertje Apr 21, 2016

colinsurprenant Apr 21, 2016

guyboertje commented Apr 21, 2016

jsvd Apr 21, 2016

colinsurprenant Apr 21, 2016

jsvd commented Apr 21, 2016

colinsurprenant Apr 21, 2016

talevy Apr 21, 2016

colinsurprenant Apr 21, 2016

colinsurprenant commented Apr 21, 2016

jsvd commented Apr 22, 2016

colinsurprenant commented Apr 25, 2016

fix for accessors lut invalidation #5132

fix for accessors lut invalidation #5132

Conversation

colinsurprenant commented Apr 18, 2016 • edited Loading

colinsurprenant commented Apr 18, 2016 • edited Loading

jsvd commented Apr 19, 2016 • edited Loading

colinsurprenant commented Apr 19, 2016

colinsurprenant commented Apr 20, 2016 • edited Loading

colinsurprenant commented Apr 20, 2016

ph commented Apr 21, 2016

guyboertje Apr 21, 2016

Choose a reason for hiding this comment

colinsurprenant Apr 21, 2016

Choose a reason for hiding this comment

guyboertje commented Apr 21, 2016

jsvd Apr 21, 2016

Choose a reason for hiding this comment

colinsurprenant Apr 21, 2016

Choose a reason for hiding this comment

jsvd commented Apr 21, 2016

colinsurprenant Apr 21, 2016

Choose a reason for hiding this comment

talevy Apr 21, 2016

Choose a reason for hiding this comment

colinsurprenant Apr 21, 2016

Choose a reason for hiding this comment

colinsurprenant commented Apr 21, 2016

jsvd commented Apr 22, 2016

colinsurprenant commented Apr 25, 2016

colinsurprenant commented Apr 18, 2016 •

edited

Loading

colinsurprenant commented Apr 18, 2016 •

edited

Loading

jsvd commented Apr 19, 2016 •

edited

Loading

colinsurprenant commented Apr 20, 2016 •

edited

Loading