Cache Parser configurers when running on a slave #2806

jerrinot · 2017-03-15T18:19:29Z

Description

This changeset caches Parser Configurer(s) when running on a slave.

Details:
applyConfiguration() invokes RPC when running on slave.
-> parsing 1000s XML files results in 1000s of RPCs.

Changelog entries

Proposed changelog entries:

Slaves cache returned cache configurers


        Cache Parser configurers when running on a slave

Reasoning: applyConfiguration() invokes RPC when running on slave. -> parsing 1000s XML files results in 1000s of RPCs.

jerrinot · 2017-03-15T18:20:35Z

There is no JIRA issue because I don't have an account yet. I created the account today but I have not received a password yet.

jerrinot · 2017-03-15T18:21:56Z

I am not sure if eternal caching is desired. I will be happy to introduce some TTL if needed.

I believe the Parser Configurers abstraction is used for TestNG tests only.

kuisathaverat · 2017-03-15T22:57:53Z

this PR could resolve https://issues.jenkins-ci.org/browse/JENKINS-10175

daniel-beck · 2017-03-16T12:29:52Z

@kuisathaverat Seems unrelated, the problem there is more likely a too permissive ant pattern/too large workspace. Notably applyConfiguration isn't in the thread dump. Why do you think this is related?

jerrinot · 2017-03-16T14:01:04Z

@daniel-beck: I agree the JENKINS-10175 looks somewhat differently. This is the stacktrace I am observing during the lengthy result processing:

"pool-1-thread-40 for CLI connection to https://hazelcast-l337.ci.cloudbees.com / waiting for hudson.remoting.Channel@6dc17b83:CLI connection to https://hazelcast-l337.ci.cloudbees.com" #182 prio=5 os_prio=0 tid=0x00007f82a8010000 nid=0x9dd05 in Object.wait() [0x00007f8362200000] 
java.lang.Thread.State: TIMED_WAITING (on object monitor) 
at java.lang.Object.wait(Native Method) 
at hudson.remoting.Request.call(Request.java:147) 
- locked <0x00000005963e5128> (a hudson.remoting.UserRequest) 
at hudson.remoting.Channel.call(Channel.java:780) 
at hudson.util.io.ParserConfigurator.applyConfiguration(ParserConfigurator.java:81) 
at hudson.tasks.junit.SuiteResult.parse(SuiteResult.java:124) 
at hudson.tasks.junit.TestResult.parse(TestResult.java:301) 
at hudson.tasks.junit.TestResult.parsePossiblyEmpty(TestResult.java:244) 
at hudson.tasks.junit.TestResult.parse(TestResult.java:175) 
at hudson.tasks.junit.TestResult.parse(TestResult.java:154) 
at hudson.tasks.junit.TestResult.<init>(TestResult.java:126) 
at hudson.tasks.junit.JUnitParser$ParseResultCallable.invoke(JUnitParser.java:132) 
at hudson.tasks.junit.JUnitParser$ParseResultCallable.invoke(JUnitParser.java:107) 
at hudson.FilePath$FileCallableWrapper.call(FilePath.java:2731) 
at hudson.remoting.UserRequest.perform(UserRequest.java:120) 
at hudson.remoting.UserRequest.perform(UserRequest.java:48) 
at hudson.remoting.Request$2.run(Request.java:332) 
at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68) 
at java.util.concurrent.FutureTask.run(FutureTask.java:266) 
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) 
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) 
at java.lang.Thread.run(Thread.java:745)

oleg-nenashev

I would like to discuss the plugin Dynamic loading case before we go forward with such approach.

P.S: This also an example of a serializable extension point, which would generally prefer to avoid. Not related to this change

oleg-nenashev · 2017-03-16T17:17:37Z

core/src/main/java/hudson/util/io/ParserConfigurator.java

-                    }
-                });
+        if (Jenkins.getInstanceOrNull() == null) {
+            all = fromCacheOrRemote();
        } else
            all = all();


brackets for else would be also useful

oleg-nenashev · 2017-03-16T17:17:37Z

core/src/main/java/hudson/util/io/ParserConfigurator.java

+
+    private static Collection<ParserConfigurator> fromCacheOrRemote() throws IOException, InterruptedException {
+        if (cache != null) {
+            return cache;


Such approach won't work reliably, e,g, if a new plugin with ParserConfigurator gets dynamically loaded. Without cache invalidation logic in may become error-prone

what would you recommend to do? can a slave subscribe to receive informations about plugins loaded on a master? or is a simple time-to-live enough?

You can use ExtensionListListener on the master side, but then how would you notify the agent to expire its cache? Could become quite tricky.

@jglick: I don't really know Jenkins codebase so I thought there could be a subscribe mechanism.

If there is no such thing then I guess a proper invalidation is difficult. So I can either use a static TTL or completely avoid using the extension mechanism

jglick

I would suggest deprecating this whole extension point as misconceived from the start, and make the junit plugin apply XMLEntityResolver on its own rather than registering it globally—it is pointless for anyone else doing parsing.

jglick · 2017-03-16T18:00:44Z

core/src/main/java/hudson/util/io/ParserConfigurator.java

@@ -55,6 +55,7 @@
 */
 public abstract class ParserConfigurator implements ExtensionPoint, Serializable {
    private static final long serialVersionUID = -2523542286453177108L;
+    private static volatile Collection<ParserConfigurator> cache;


@Extensions should generally not be cached in a static field, lest the wrong objects carry over from one Jenkins session to the next. If I understand correctly, this field is only used when this class is loaded in an agent JVM, which would be specific to one session, so it is not a problem.

jglick · 2017-03-16T18:00:44Z

core/src/main/java/hudson/util/io/ParserConfigurator.java

+
+    private static Collection<ParserConfigurator> fromCacheOrRemote() throws IOException, InterruptedException {
+        if (cache != null) {
+            return cache;


You can use ExtensionListListener on the master side, but then how would you notify the agent to expire its cache? Could become quite tricky.

jglick · 2017-03-16T18:02:05Z

The reported stack traces in JENKINS-10175 are unrelated to this optimization.

jerrinot · 2017-03-16T18:38:43Z

@oleg-nenashev, @jglick: thanks a lot for your review. Would something like this work better for you?

kuisathaverat · 2017-03-16T19:36:39Z

@daniel-beck I have supposed that the next step of ParserConfigurator should be the parsing process and depends on of when you take the thread dump you could find the stack trace that @jerrinot has posted, also the behavior it is pretty similar.

kuisathaverat · 2017-03-17T13:52:30Z

@jglick @oleg-nenashev on this https://github.com/jenkinsci/junit-plugin/pull/65/files makes the change to XMLEntityResolver for the JUnit Plugin, it is for sure better than a change on core

batmat · 2018-12-28T23:23:12Z

This PR is now conflicted.

Given the last comment was in March 2017, I'm going to propose closing it. I will close this, if nobody disagrees, on the next pass, likely next week.

Thanks!

batmat · 2019-01-17T16:00:53Z

Closing, as per my last comment.

@jerrinot please feel free to comment here anytime once/if you feel like getting back to this, and thanks again for the work. We're all happy to reopen anytime it's needed.

Thanks!

jerrinot · 2019-01-17T16:07:14Z

@batmat: this PR was actually superseded by jenkinsci/junit-plugin#65, I just forgot to close it. sorry, for that!

batmat · 2019-01-17T16:19:09Z

No worries, thanks again for your involvement!

Cache Parser configurers when running on a slave

Loading status checks…

b2adf07

Reasoning: applyConfiguration() invokes RPC when running on slave. -> parsing 1000s XML files results in 1000s of RPCs.

oleg-nenashev reviewed Mar 16, 2017

View changes

oleg-nenashev added needs-more-reviews on-hold labels Mar 16, 2017

jglick requested changes Mar 16, 2017

View changes

jerrinot mentioned this pull request Mar 16, 2017

Do not register XMLEntityResolver as a global extension #65

Merged

batmat added the unresolved-merge-conflict label Dec 28, 2018

batmat added the proposed-for-close label Dec 28, 2018

batmat closed this Jan 17, 2019

jerrinot deleted the jerrinot:improvements/cache-parser-configurers branch Jan 17, 2019

jglick mentioned this pull request Jan 17, 2019

Deprecating ParserConfigurator #3852

Merged

jenkinsci / jenkins

Cache Parser configurers when running on a slave #2806

Cache Parser configurers when running on a slave #2806

jerrinot commented Mar 15, 2017

jerrinot commented Mar 15, 2017

jerrinot commented Mar 15, 2017 •

edited

kuisathaverat commented Mar 15, 2017 •

edited

daniel-beck commented Mar 16, 2017

jerrinot commented Mar 16, 2017

oleg-nenashev left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jglick left a comment

This comment has been minimized.

This comment has been minimized.

jglick commented Mar 16, 2017

jerrinot commented Mar 16, 2017 •

edited

kuisathaverat commented Mar 16, 2017

kuisathaverat commented Mar 17, 2017 •

edited

batmat commented Dec 28, 2018

batmat commented Jan 17, 2019

jerrinot commented Jan 17, 2019

batmat commented Jan 17, 2019

jenkinsci / jenkins

Sponsor jenkinsci/jenkins

Join GitHub today

Cache Parser configurers when running on a slave #2806

Cache Parser configurers when running on a slave #2806

Conversation

jerrinot commented Mar 15, 2017

Description

Changelog entries

jerrinot commented Mar 15, 2017

jerrinot commented Mar 15, 2017 • edited

kuisathaverat commented Mar 15, 2017 • edited

daniel-beck commented Mar 16, 2017

jerrinot commented Mar 16, 2017

oleg-nenashev left a comment

This comment has been minimized.

oleg-nenashev Mar 16, 2017

This comment has been minimized.

oleg-nenashev Mar 16, 2017

This comment has been minimized.

jerrinot Mar 16, 2017 • edited

This comment has been minimized.

jglick Mar 16, 2017

This comment has been minimized.

jerrinot Mar 16, 2017 • edited

jglick left a comment

This comment has been minimized.

jglick Mar 16, 2017

This comment has been minimized.

jglick Mar 16, 2017

jglick commented Mar 16, 2017

jerrinot commented Mar 16, 2017 • edited

kuisathaverat commented Mar 16, 2017

kuisathaverat commented Mar 17, 2017 • edited

batmat commented Dec 28, 2018

batmat commented Jan 17, 2019

jerrinot commented Jan 17, 2019

batmat commented Jan 17, 2019

jerrinot commented Mar 15, 2017 •

edited

kuisathaverat commented Mar 15, 2017 •

edited

jerrinot Mar 16, 2017 •

edited

jerrinot Mar 16, 2017 •

edited

jerrinot commented Mar 16, 2017 •

edited

kuisathaverat commented Mar 17, 2017 •

edited