Move class caching from ComputeStepSyntaxElement to CompiledPipeline #11482

colinsurprenant · 2020-01-09T22:44:52Z

Fixes #11105
Relates to #11175

This is an alternate implementation of #11479 where the compiles class caching is kept per-pipeline into the CompiledPipeline class.

See #11479 for the problem details.

This approach has the following benefits:

Solves a potential leak for a global cache upon pipeline reloads.
Solves a potential class name collision for identical plugins IDs across multiple pipelines.
Avoids dealing with cache invalidation altogether because the cache is cleared every time a pipeline is created/reloaded.

colinsurprenant · 2020-01-09T22:58:46Z

logstash-core/src/main/java/org/logstash/config/ir/CompiledPipeline.java

                    leaf -> outputDataset(leaf, flatten(Collections.emptyList(), leaf))
                ).collect(Collectors.toList()));
            }
        }
+        /**


This is actually just moved code from DatasetCompiler here to keep consistency with the other filterDataset, outputDataset, ... methods.

yaauie

I think this solves the problem, and does so more elegantly than the other PR.

I've added a couple of suggestions in-line, with links to branches implementing what I talk about.

It would be nice if we could also find a way to avoid synchronizing on COMPILER, since this means only a single thread can be working on this at a time, but I don't understand exactly what is going on with how our single shared COMPILER is being mutated.

logstash-core/src/main/java/org/logstash/config/ir/CompiledPipeline.java

yaauie · 2020-01-10T02:47:24Z

logstash-core/src/main/java/org/logstash/config/ir/CompiledPipeline.java

@@ -119,6 +137,15 @@ public CompiledPipeline(
     * @return Compiled {@link Dataset} representation of the underlying {@link PipelineIR} topology
     */
    public Dataset buildExecution() {
+        synchronized (this) {


synchronization could be avoided by using a final AtomicReference<CompiledExecution> warmedCompiledExecution that is primed with the warm compiled execution: yaauie@017051f

Yes, I like that, good suggestion.

logstash-core/src/main/java/org/logstash/config/ir/CompiledPipeline.java

logstash-core/src/main/java/org/logstash/config/ir/compiler/ComputeStepSyntaxElement.java

colinsurprenant · 2020-01-10T22:17:16Z

@yaauie Thanks for the review; all great suggestions.

For the COMPILER synchronization, I would prefer not to remove it just yet as I am also not 100% sure if it is still needed and for exactly what. Also this is not central to the goal of this PR and we can followup on this as a potential extra optimization in a new issue. WDYT?

yaauie · 2020-01-10T22:30:54Z

I think we can avoid a global lock compiling java classes with yaauie@a8a5da1

From what I can tell, the single SimpleCompiler COMPILER was hacked to be reused by setting its parent classloader to be the current classloader (which is the result of the most recent cook), nearly-ensuring that getting the current class loader would always be able to load the class that it had just cooked even if another thread had cooked another input in the meantime. The only reason why this hack could have worked in practice was because we happened to already be synchronized on COMPILER for other reasons.

yaauie · 2020-01-10T22:31:43Z

I'm glad to follow up with lock removal in a later PR. This one should remove significant rework anyway.

colinsurprenant · 2020-01-10T22:42:43Z

@yaauie Included your suggestions. Great stuff figuring the COMPILER lock 💪

colinsurprenant · 2020-01-10T23:49:33Z

unrelated build error; restarting.

colinsurprenant · 2020-01-10T23:49:42Z

jenkins test this

yaauie · 2020-01-11T01:28:46Z

I think we should be good-to-go here, but would you mind letting it sit over the weekend before merging so I can continue ruminating?

yaauie

Great work, @colinsurprenant 🎉

LGTM 👍

colinsurprenant · 2020-01-13T16:57:49Z

Thanks @yaauie - great review, as usual 👍

colinsurprenant · 2020-01-13T17:54:08Z

Merged in master, backports for 7.6.0 and 7.5.2.

colinsurprenant · 2020-02-01T03:54:22Z

This PR addressed the multiple workers slowdown but introduced in 7.5.2 a regression for the multiple pipelines use-case. This PR will be reverted and replaced by #11564 which should correctly solve both problems.

[7.6 backport of #11564]

[7.5 backport of #11564]

colinsurprenant · 2020-02-04T19:41:28Z

The regression has been addressed by #11564 and will be available in 7.6 (and 7.5.3 if such a version is released). Note that the "new" fix is slightly less efficient than this one but still much much more efficient than 7.5.1 and does not slow down the multi-pipelines use-case.

7.7 backport of #11564

colinsurprenant mentioned this pull request Jan 9, 2020

Cache compiled classes by vertex ID #11479

Closed

colinsurprenant commented Jan 9, 2020

View reviewed changes

This was referenced Jan 9, 2020

Start time much longer after upgrade from 6.x to 7.x #11105

Closed

[meta] Pipeline Java Execution important issues #11175

Closed

yaauie reviewed Jan 10, 2020

View reviewed changes

yaauie approved these changes Jan 13, 2020

View reviewed changes

cache compiled datasets

f8e895f

colinsurprenant force-pushed the compilation branch from bfb60cc to f8e895f Compare January 13, 2020 17:07

This was referenced Jan 13, 2020

[7.5 clean backport of #11482] cache compiled datasets #11490

Merged

[7.6 clean backport of #11482] cache compiled datasets #11491

Merged

colinsurprenant merged commit 4b29112 into elastic:master Jan 13, 2020

colinsurprenant added bug v8.0.0 labels Jan 13, 2020

colinsurprenant added a commit that referenced this pull request Jan 13, 2020

[7.5 clean backport of #11482] cache compiled datasets (#11490)

af89d16

colinsurprenant added a commit that referenced this pull request Jan 13, 2020

[7.6 clean backport of #11482] cache compiled datasets (#11491)

fc152d3

colinsurprenant added the Java Execution label Jan 13, 2020

colinsurprenant mentioned this pull request Jan 13, 2020

Proposal: Eager start of inputs #11493

Open

yaauie mentioned this pull request Jan 14, 2020

JEE: remove global compiler lock #11503

Draft

colinsurprenant added a commit to colinsurprenant/logstash that referenced this pull request Feb 1, 2020

revert elastic#11482 and fix redudant code generation

32e7bac

colinsurprenant added a commit to colinsurprenant/logstash that referenced this pull request Feb 1, 2020

revert elastic#11482 and fix redundant code generation

4adb0e2

colinsurprenant mentioned this pull request Feb 1, 2020

revert #11482 and fix redundant code generation #11564

Merged

colinsurprenant added a commit to colinsurprenant/logstash that referenced this pull request Feb 4, 2020

revert elastic#11482 and fix redundant code generation

2f888d4

colinsurprenant added a commit to colinsurprenant/logstash that referenced this pull request Feb 4, 2020

revert elastic#11482 and fix redundant code generation

74ecad0

colinsurprenant added a commit to colinsurprenant/logstash that referenced this pull request Feb 4, 2020

revert elastic#11482 and fix redundant code generation

4206d3b

colinsurprenant added a commit to colinsurprenant/logstash that referenced this pull request Feb 4, 2020

revert elastic#11482 and fix redundant code generation

18fd1dc

colinsurprenant added a commit that referenced this pull request Feb 4, 2020

revert #11482 and fix redundant code generation (#11564)

8481bd0

colinsurprenant added a commit that referenced this pull request Feb 4, 2020

revert #11482 and fix redundant code generation (#11574)

402f02d

[7.6 backport of #11564]

colinsurprenant added a commit that referenced this pull request Feb 4, 2020

revert #11482 and fix redundant code generation (#11575)

84b1bf6

[7.5 backport of #11564]

colinsurprenant added a commit that referenced this pull request Feb 7, 2020

revert #11482 and fix redundant code generation (#11576)

72b5f29

7.7 backport of #11564

colinsurprenant mentioned this pull request Apr 3, 2020

warm pipeline compilation cache #11750

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move class caching from ComputeStepSyntaxElement to CompiledPipeline #11482

Move class caching from ComputeStepSyntaxElement to CompiledPipeline #11482

colinsurprenant commented Jan 9, 2020

colinsurprenant Jan 9, 2020

yaauie left a comment

yaauie Jan 10, 2020

colinsurprenant Jan 10, 2020

colinsurprenant Jan 10, 2020

colinsurprenant commented Jan 10, 2020

yaauie commented Jan 10, 2020 •

edited

Loading

yaauie commented Jan 10, 2020

colinsurprenant commented Jan 10, 2020

colinsurprenant commented Jan 10, 2020

colinsurprenant commented Jan 10, 2020

yaauie commented Jan 11, 2020

yaauie left a comment

colinsurprenant commented Jan 13, 2020

colinsurprenant commented Jan 13, 2020

colinsurprenant commented Feb 1, 2020

colinsurprenant commented Feb 4, 2020

Move class caching from ComputeStepSyntaxElement to CompiledPipeline #11482

Move class caching from ComputeStepSyntaxElement to CompiledPipeline #11482

Conversation

colinsurprenant commented Jan 9, 2020

colinsurprenant Jan 9, 2020

Choose a reason for hiding this comment

yaauie left a comment

Choose a reason for hiding this comment

yaauie Jan 10, 2020

Choose a reason for hiding this comment

colinsurprenant Jan 10, 2020

Choose a reason for hiding this comment

colinsurprenant Jan 10, 2020

Choose a reason for hiding this comment

colinsurprenant commented Jan 10, 2020

yaauie commented Jan 10, 2020 • edited Loading

yaauie commented Jan 10, 2020

colinsurprenant commented Jan 10, 2020

colinsurprenant commented Jan 10, 2020

colinsurprenant commented Jan 10, 2020

yaauie commented Jan 11, 2020

yaauie left a comment

Choose a reason for hiding this comment

colinsurprenant commented Jan 13, 2020

colinsurprenant commented Jan 13, 2020

colinsurprenant commented Feb 1, 2020

colinsurprenant commented Feb 4, 2020

yaauie commented Jan 10, 2020 •

edited

Loading