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

[JENKINS-48349] - Cache permission id to avoid allocating of new strings #3155

Merged
merged 1 commit into from Nov 26, 2017

Conversation

@Jimilian
Copy link
Contributor

commented Nov 21, 2017

Every request that comes from Jelly is checked against Permissions.
As result it leads to a call of getId method that produces the new string.
Usually it's not a problem, but in case of stop-the-world pause user requests are accumulated.
So, once pause is finished, we forcibly allocated tons of strings for
every request. That leads to new stop-the-world pause. (And this cycle
can repeat multiple times)

Now to help Jenkins to recover we need to turn off nginx for 1 minute :(

Proposed changelog entries

  • Performance: Jenkins can recover faster after FullGC/stop-the-world pause

Desired reviewers

@svanoort, @oleg-nenashev, @jglick, @daniel-beck

Screenshots that show memory allocations in 1.5 seconds just after stop-the-world pause:
screen shot 2017-11-21 at 15 12 05
screen shot 2017-11-21 at 15 09 33

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

👍

@@ -222,7 +225,7 @@ public boolean isContainedBy(@Nonnull PermissionScope s) {
* @see #fromId(String)
*/
public @Nonnull String getId() {
return owner.getName()+'.'+name;
return id;

This comment has been minimized.

Copy link
@svanoort

svanoort Nov 21, 2017

Member

Are Permission objects ever serialized in configuration? Generally they're statically created AFAIK but if a plugin lets you select the permission type to apply to some property (none I know of off the top of my head) then we could have cases where this is null.

If so, we should probably check if field is non-null and fall back to old logic when null.

Copy link
Member

left a comment

Great catch here @Jimilian -- I think we do a lot of needless object allocation in Jenkins in general and removing some of that will help with performance. Especially when the method is called frequently -- this will help with more efficient CPU cache use potentially.

I do think we probably should include the null-check on the field though, to protect against some obscure usage that serializes a permission (maybe picking the Permission to apply for some feature from a list).

Maybe being overly cautious here, but for pure optimizations, best to limit potential negative impact.

Every request that comes from Jelly is checked against Permissions.
As result it leads to a call of `getId` method that produces the new string.
Usually it's not a problem, but in case of stop-the-world pause user requests are accumulated.
So, once pause is finished, we forcibly allocated tons of strings for
every request. That leads to new stop-the-world pause. (And this cycle
can repeat multiple times)
@Jimilian Jimilian force-pushed the Jimilian:cache_permission_id branch from f0d9264 to b2c40cb Nov 22, 2017
@Jimilian

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2017

@svanoort, addressed.

Copy link
Member

left a comment

@Jimilian I LIKE IT. Now that my one concern is addressed, let's merge this! 😄

@oleg-nenashev oleg-nenashev merged commit b431eb4 into jenkinsci:master Nov 26, 2017
1 check passed
1 check passed
continuous-integration/jenkins/pr-head This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Dec 3, 2017

@Jimilian has created a placeholder issue for it: https://issues.jenkins-ci.org/browse/JENKINS-48349.

@oleg-nenashev oleg-nenashev changed the title Cache permission id to avoid allocating of new strings [JENKINS-48349] - Cache permission id to avoid allocating of new strings Dec 3, 2017
@Jimilian Jimilian deleted the Jimilian:cache_permission_id branch Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.