dynamically get the plugin id instead of hardcoding it in the generated classes by colinsurprenant · Pull Request #12038 · elastic/logstash

colinsurprenant · 2020-06-18T21:10:46Z

Fixes #12031

This PR fixes the regression introduced in 7.7.0 where the plugin id was set in the thread context by hard coding the unique plugin id in the generated class resulting in generating non cacheable classes that all contained a unique id.

The fix is to dynamically get the plugin id from the plugin instance field which result in non-unique generated code and thus allowing the reuse of equivalent generated classes.

Before this PR the generated filter and output classes would contain code similar to

org.apache.logging.log4j.ThreadContext.put("plugin.id", "ce80c1c692c7fd1664e67aa37b9b5ee8f6d7fa336b22bad5b14684ca44132adc");

This PR make that code be like:

org.apache.logging.log4j.ThreadContext.put("plugin.id", field1.getId().toString());

andsel · 2020-06-23T09:23:36Z

This LGTM. I've added a test to check if we experiment performance regressions future. /cc @jsvd

jsvd · 2020-06-23T16:15:34Z

Test has failed:

17:10:22 org.logstash.config.ir.CompiledPipelineTest > testCompilerCacheCompiledClasses FAILED
17:10:22     java.lang.AssertionError: expected:<1> but was:<0>
17:10:22         at org.junit.Assert.fail(Assert.java:88)
17:10:22         at org.junit.Assert.failNotEquals(Assert.java:834)
17:10:22         at org.junit.Assert.assertEquals(Assert.java:645)
17:10:22         at org.junit.Assert.assertEquals(Assert.java:631)
17:10:22         at org.logstash.config.ir.CompiledPipelineTest.testCompilerCacheCompiledClasses(CompiledPipelineTest.java:590)

andsel · 2020-06-23T16:21:25Z

@jsvd I think the cache is already warmed up by other tests

jsvd · 2020-06-23T16:39:12Z

good catch, @andsel can you open an issue to discuss the implementation of the cache as a static? as it is it's difficult to configure/disable the cache in testing.
In the meantime we can simply evict the cache before tests in this class.

andsel · 2020-06-23T16:52:43Z

Added issue #12044 to improve the internal class cache

jsvd · 2020-06-24T16:24:28Z

+    static {
+        COMPILER.setDebuggingInformation(true, true, true);
+    }
+


I don't believe this was intended to stay?

Suggested change

static {

COMPILER.setDebuggingInformation(true, true, true);

}

I was thinking the same, but if the switch for Janino debug is on, probably having the right line reference on the generated sources could be helpful.

Do you think could impact negatively the compilation part?

I haven't tested. can't this be enabled through JAVA_OPTS?

I found it, it's org.codehaus.janino.source_debugging.enable so removed the code fragment

jsvd · 2020-06-24T16:29:57Z

+// for 7.6
+//            return new FilterDelegatorExt(
+//                    RubyUtil.RUBY, RubyUtil.FILTER_DELEGATOR_CLASS)
+//                    .initForTesting(this.filter.get());


we won't be backporting this to 7.6

Suggested change

// for 7.6

// return new FilterDelegatorExt(

// RubyUtil.RUBY, RubyUtil.FILTER_DELEGATOR_CLASS)

// .initForTesting(this.filter.get());

yes to remote, my bad

jsvd · 2020-06-24T16:30:59Z

+        final CompiledPipeline compiledPipeline2 = new CompiledPipeline(pipeline2, pluginFactory);
+        compiledPipeline2.buildExecution();
+        final int cachedAfter = ComputeStepSyntaxElement.classCacheSize();
+        System.out.println("DNADBG>> cachedAfter: " + cachedAfter + ", cachedBefore: " + cachedBefore);


what's DNADBG?

ehm, comment to remove, my bad

jsvd · 2020-06-25T08:54:34Z

let's avoid these refactors, I think we benefit from having an explicit list, makes it easier to understand the dependencies. Intellij can be told to stop doing this.

…ead of hardcode Changed ComputeStepSyntaxElement to generate Java code to retrieve the plugin's id by a method instead of hardcoding the value in the generated code. This permit to share more compiled classes, that differs only by plugin.id and speed up the pipeline compilation. The change has been secured by future regression with unit test that track pipeline compilations times. Co-authored-by: Andrea Selva <andsel@users.noreply.github.com> Co-authored-by: João Duarte <jsvd@users.noreply.github.com> Fixes: elastic#12031

elasticsearch-bot · 2020-06-25T13:09:40Z

Andrea Selva merged this to master!

Pull request was also backported into the following branches:

Branch	Commits
7.x	`d767848`
7.8	`84f9154`

with PR elastic#11773 some configuration code part was moved from Ruby to Java, due to this move the CompiledPipelineTest.verifyRegex method changed the type of thrown exception from stricter IncompleteSourceWithMetadataException to coarse parent InvalidIRException. With changes in PR elastic#12038 some import cleanup was done and that removed the import of IncompleteSourceWithMetadataException and necessity of this fix.

with PR #11773 some configuration code part was moved from Ruby to Java, due to this move the CompiledPipelineTest.verifyRegex method changed the type of thrown exception from stricter IncompleteSourceWithMetadataException to coarse parent InvalidIRException. With changes in PR #12038 some import cleanup was done and that removed the import of IncompleteSourceWithMetadataException and necessity of this fix.

colinsurprenant · 2020-06-25T14:42:04Z

Thanks @jsvd for the review and @andsel for moving this to completion with the added tests. 🚀

colinsurprenant added the performance regression label Jun 18, 2020

colinsurprenant mentioned this pull request Jun 18, 2020

Pipeline compilation significantly increased in 7.7.0 in comparison to 7.6.2 #12031

Closed

kares mentioned this pull request Jun 23, 2020

Start time much longer for multiple pipelines #12020

Closed

jsvd reviewed Jun 23, 2020

View reviewed changes

Comment thread logstash-core/src/main/java/org/logstash/config/ir/compiler/DatasetCompiler.java

jsvd reviewed Jun 23, 2020

View reviewed changes

Comment thread logstash-core/src/test/java/org/logstash/config/ir/CompiledPipelineTest.java Outdated

yaauie mentioned this pull request Jun 23, 2020

Avoid class loader chaining in janino compilation #12045

Closed

jsvd mentioned this pull request Jun 24, 2020

JEE: nix global compiler lock by normalizing to BaseDataset interface #12047

Closed

andsel requested a review from jsvd June 24, 2020 16:20

jsvd reviewed Jun 24, 2020

View reviewed changes

andsel force-pushed the dynamic_plugin_id branch from 6ff92cb to f7627fc Compare June 25, 2020 08:44

jsvd reviewed Jun 25, 2020

View reviewed changes

Comment thread logstash-core/src/test/java/org/logstash/config/ir/CompiledPipelineTest.java Outdated

andsel force-pushed the dynamic_plugin_id branch from 3de61df to 9ac8846 Compare June 25, 2020 12:22

andsel force-pushed the dynamic_plugin_id branch from 9ac8846 to 1a11abd Compare June 25, 2020 13:08

elasticsearch-bot merged commit ffac2df into elastic:master Jun 25, 2020

karenzone mentioned this pull request Jun 26, 2020

Doc:Add performance known issue to release notes #12035

Open

colinsurprenant added v7.8.1 v7.9.0 v8.0.0 labels Jul 21, 2020

yaauie mentioned this pull request Jul 31, 2020

Config compilation is still running after 1 hour #12156

Closed

	static {
	COMPILER.setDebuggingInformation(true, true, true);
	}

Conversation

colinsurprenant commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andsel commented Jun 23, 2020

Uh oh!

Uh oh!

Uh oh!

jsvd commented Jun 23, 2020

Uh oh!

andsel commented Jun 23, 2020

Uh oh!

jsvd commented Jun 23, 2020

Uh oh!

andsel commented Jun 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andsel Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearch-bot commented Jun 25, 2020

Uh oh!

colinsurprenant commented Jun 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

colinsurprenant commented Jun 18, 2020 •

edited

Loading

andsel Jun 24, 2020 •

edited

Loading