Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Bytecode Compatibility Transformer #5526

Merged
merged 4 commits into from Jun 1, 2021
Merged

Remove Bytecode Compatibility Transformer #5526

merged 4 commits into from Jun 1, 2021
+1 −55

Conversation

basil
Copy link
Member

@basil basil commented May 25, 2021 •
edited

Commit 47de54d introduced Bytecode Compatibility Transformer to allow core to do signature changes on fields that plugins might depend on. This functionality was never enabled for plugins, so there are no plugins with org.jenkinsci.bytecode.AdaptField annotations. The only org.jenkinsci.bytecode.AdaptField annotations are in core (I verified this with usage-in-plugins and searching for AdaptField in sources), and the vast majority of them have been deleted over the years. Only two fields with such annotations remain: hudson.model.Queue$Item#id and hudson.model.AbstractProject#triggers. Running usage-in-plugins against

hudson.model.Queue.Item#id
hudson.model.Queue$Item#id
hudson.model.AbstractProject#triggers

turned up exactly two references, both to hudson/model/Queue$Item#id:

  • Slave Prerequisites, a plugin with 517 installs last released 9 years ago
  • vertx, a plugin with 8 installs last released 9 years ago

Let's consider the cost of maintaining Bytecode Compatibility Transformer:

  • Roughly ~2000 lines of non-trivial Java code has to be maintained.
  • A dependency on a forked/repackaged version of ASM 6 has to be maintained. (Nothing else in the ecosystem uses this repackaged version of ASM 6, as I also verified with usage-in-plugins and by searching in sources.)
  • There is no realistic test coverage in Jenkins core for this compatibility scenario, making maintenance harder.

Now let's consider the benefit of maintaining Bytecode Compatibility Transformer:

  • Two plugins last released 9 years ago can access an integer hudson/model/Queue$Item#id field.

In my humble opinion, it is time to dispense with this subsystem.

Proposed changelog entries

Removed: The bytecode transformer that retains compatibility after changing public Java APIs and its associated hudson.ClassicPluginStrategy.noBytecodeTransformer system property have been removed.

Developer: Plugins that rely on the hudson.model.Queue$Item#id or hudson.model.AbstractProject#triggers fields, including Slave Prerequisites and vertx, must be updated to call the corresponding getters.

Proposed upgrade guidelines

Support for plugins that rely on the hudson.model.Queue$Item#id or hudson.model.AbstractProject#triggers fields, including Slave Prerequisites and vertx, has been dropped. Any such plugins must be removed prior to upgrading Jenkins. If you have customized the hudson.ClassicPluginStrategy.noBytecodeTransformer system property, you should remove this customization.

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@basil basil added dependencies Pull requests that update a dependency file developer Changes which impact plugin developers upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed labels May 25, 2021
@daniel-beck
Copy link
Member

daniel-beck commented May 25, 2021 •
edited

For the specific fields it seems useful. I wonder about other fields in the future though. There's some stuff in Jenkins in particular I've recently seen that may benefit from this feature to make sure callers access methods rather than public fields.

OTOH if we'd never again use this feature due to the pain it's caused in the past, we can just remove it.

@basil
Copy link
Member Author

basil commented May 25, 2021

OTOH if we'd never again use this feature due to the pain it's caused in the past, we can just remove it.

I do not think we would ever want to use this again. It is unnecessarily complex and opaque, lacks automated testing, and does not interoperate well with debuggers and other IDE tooling.

  • Commit 47de54d wrote: "Let's see for a while until we open this functionality up to plugins." Well, it's been a while, and we still haven't opened this functionality up to plugins, so that is not a good sign.
  • Judging from commit 710f168 this never worked all that well to begin with.
  • Use of Bytecode Compatibility Transformer was considered and rejected in JEP 227.

I think it is time to move on.

@basil basil added the removed This PR removes a feature or a public API label May 25, 2021
@daniel-beck daniel-beck requested a review from jglick May 25, 2021 16:06
@basil basil added the squash-merge-me Unclean or useless commit history, should be merged only with squash-merge label May 25, 2021
@jglick jglick requested a review from a team May 25, 2021 21:07
jglick
jglick approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I would love to also get rid of @WithBridgeMethods but at least that runs at build time so you can see what is going on in bytecode (still awkward for IDEs).

basil added a commit to basil/update-center2 that referenced this pull request May 25, 2021
@basil
Formally deprecating plugins left behind in jenkinsci/jenkins#5320, j…
df2ac1e 
…enkinsci/jenkins#5338, jenkinsci/jenkins#5521, and jenkinsci/jenkins#5526.
@basil basil mentioned this pull request May 25, 2021
Merged
@jglick
Copy link
Member

jglick commented May 26, 2021

I suspect this also improves Jenkins startup time when lots of plugins are enabled. The transformer has short-circuit logic to avoid processing a byte[] if it does not seem to be using the annotated fields. Unfortunately even this logic involves doing a full parse of the constant pool, meaning some nontrivial array scanning and object creation. If we wanted to keep this transformer, we would want it to take the list of annotated classes and fields in binary format, like (hudson/model/Queue$Item | hudson/model/AbstractProject) & (id | triggers), then do something fancy like the Commentz-Walter algorithm to avoid any more than trivial overhead on the majority of classes which do not match this first pass, before continuing to the constant pool scan and the next bypass.

@timja timja requested a review from a team May 26, 2021 12:04
@basil
Copy link
Member Author

basil commented May 26, 2021

I suspect this also improves Jenkins startup time when lots of plugins are enabled.

I did some relatively unscientific tests with the set of plugins used at my company (133 plugins). After starting Jenkins once with this plugin set, I measured how long Jenkins took to get from cold startup to "Jenkins is fully up and running". Before this change, the results were 17.80s, 18.05s, 18.30s, 18.57s, and 19.23s (average 18.39s). After this change the results were 17.27s, 17.31s, 17.50s, 17.63s, and 18.29s (average 17.6s). So I guess I shaved off about a second on average. I'll take it!

@jglick
Copy link
Member

jglick commented May 26, 2021

Not sure if that is statistically significant or not, but by inspection this change reduces startup time by some amount. Thanks!

@timja timja added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 29, 2021
@timja
Copy link
Member

timja commented May 29, 2021

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

oleg-nenashev
oleg-nenashev approved these changes May 29, 2021
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections from me. I am not sure what is the impact on various 3rd party and proprietary plugin. In any case, BCT is very difficult to use and maintain. Let's take a risk. In the worst case we will be able to revert the dependency or create an API plugin for it if possible

@res0nance res0nance merged commit 2760e3e into jenkinsci:master Jun 1, 2021
@basil basil deleted the bytecode-compatibility-transformer branch June 1, 2021 13:00
@jglick
Copy link
Member

jglick commented Jun 1, 2021

create an API plugin for it

Does not make sense.

@jtnord jtnord mentioned this pull request Jun 11, 2021
Merged
daniel-beck pushed a commit to jenkins-infra/update-center2 that referenced this pull request Jun 21, 2021
Formally deprecating plugins left behind in recent core PRs (#525)
9812b10 
Formally deprecating plugins left behind in jenkinsci/jenkins#5320, jenkinsci/jenkins#5338, jenkinsci/jenkins#5521, and jenkinsci/jenkins#5526.
@basil basil mentioned this pull request Jul 5, 2021
Merged
@basil basil mentioned this pull request Aug 27, 2021
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jglick jglick jglick approved these changes

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@timja timja timja approved these changes

@res0nance res0nance res0nance approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback removed This PR removes a feature or a public API squash-merge-me Unclean or useless commit history, should be merged only with squash-merge upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed
Projects
None yet
Milestone
No milestone
6 participants
@basil @daniel-beck @jglick @timja @oleg-nenashev @res0nance