[JENKINS-35098] Disable AutoBrowserHolder by default to improve the changelog rendering performance #2371
Conversation
|
I'm not convinced the AutoBrowser holder is useless at all.
|
oleg-nenashev
added
the
needs-more-reviews
|
🐝 👍 let's remove this "magic" |
Not so easy. This method takes both the “owner” SCM, and each candidate SCM. And the performance problem to begin with is finding those candidate SCMs. So you cannot actually call the method. The only thing you could do is use reflection to check if it is overridden. Anyway this would only potentially benefit Subversion users (i.e., they would get the existing feature, at the existing performance cost). CVS users would continue to get the broken implementation, and the performance cost.
That would leave most users with the performance problem. You could have a system property to enable the feature, but by the time you figure out that you need this feature and such a property exists and you modify your Jenkins startup script and restart, you could have just fixed your job configurations already.
I did not say it was useless, just that on balance it seems like a poor decision from the start to have put this in core. We do not have anything else that I know of in Jenkins which quietly guesses what you meant to configure without you actually doing so. For people using traditional GUI configuration of jobs, this class of feature can be helpful, but the UI would probably need to be different: say, a list of suggested configuration tweaks to newly created jobs to set up not just the SCM browser but the |
|
@jglick |
oleg-nenashev
added
the
needs-comments-resolved
|
Still prefer to delete it altogether and simplify the code, but if you insist… |
…ot using it by default.
jglick
removed
the
needs-comments-resolved
| @@ -85,6 +85,15 @@ | |||
| @ExportedBean | |||
| public abstract class SCM implements Describable<SCM>, ExtensionPoint { | |||
|
|
|||
| /** JENKINS-35098: discouraged */ | |||
| @SuppressWarnings("FieldMayBeFinal") | |||
| private static boolean useAutoBrowserHolder = Boolean.getBoolean(SCM.class.getName() + ".useAutoBrowserHolder"); | |||
There was a problem hiding this comment.
Please use SystemProperties.getBoolean() (new API in 2.4)
|
🐝 from me, @reviewbybees done |
jglick
added
ready-for-merge
oleg-nenashev
changed the title
| SCMDescriptor<?> d = SCM._for(target).get(scmidx); | ||
| d.generation++; | ||
| return d.newInstance(req, req.getSubmittedForm().getJSONObject("scm")); | ||
| SCM scm = req.bindJSON(SCM.class, req.getSubmittedForm().getJSONObject("scm")); |
There was a problem hiding this comment.
@oleg-nenashev @jglick Could this have caused JENKINS-35906?
There was a problem hiding this comment.
Probably. No longer calling SCMDescriptor.newInstance overloads. They are a bad idea of course and unnecessary.
There was a problem hiding this comment.
Possibly could use DescriptorExtensionList.newInstanceFromRadioList for this purpose.
…hangelog rendering performance (jenkinsci#2371) * [FIXED JENKINS-35098] Deleting AutoBrowserHolder. * Normalizing form binding scheme for AbstractProject.scm. * At @oleg-nenashev’s insistence, restoring AutoBrowserHolder, though not using it by default. * Using SystemProperties at @oleg-nenashev’s recommendation.
…hangelog rendering performance (jenkinsci#2371) * [FIXED JENKINS-35098] Deleting AutoBrowserHolder. * Normalizing form binding scheme for AbstractProject.scm. * At @oleg-nenashev’s insistence, restoring AutoBrowserHolder, though not using it by default. * Using SystemProperties at @oleg-nenashev’s recommendation.
…hangelog rendering performance (#2371) * [FIXED JENKINS-35098] Deleting AutoBrowserHolder. * Normalizing form binding scheme for AbstractProject.scm. * At @oleg-nenashev’s insistence, restoring AutoBrowserHolder, though not using it by default. * Using SystemProperties at @oleg-nenashev’s recommendation. (cherry picked from commit d33df0f)
JENKINS-35098
See JIRA for motivation.
Also took the opportunity to clean up some ancient form binding. The fewer weird tricks we have floating around the source base, the better.
@reviewbybees