Skip to content
Permalink
Browse files
[FIXED JENKINS-19392] Simpler and more efficient to use String.intern…
… to compact fingerprint data.

This should work across builds and even across jobs, and does not force us to load other builds.
  • Loading branch information
jglick committed Dec 22, 2014
1 parent 47fa17b commit 2f59ffc475dc73a9a9cd5a7596819f3242c22f6f
Showing with 11 additions and 38 deletions.
  1. +3 −0 changelog.html
  2. +8 −38 core/src/main/java/hudson/tasks/Fingerprinter.java
@@ -55,6 +55,9 @@
<!-- Record your changes in the trunk here. -->
<div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
<ul class=image>
<li class=bug>
Fingerprint compaction aggravated lazy-loading performance issues.
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-19392">issue 19392</a>)
<li class=bug>
Possible unreleased workspace lock if SCM polling fails during setup.
(<a href="https://issues.jenkins-ci.org/browse/JENKINS-23568">issue 23568</a>)
@@ -294,8 +294,7 @@ public boolean isApplicable(Class<? extends AbstractProject> jobType) {

public FingerprintAction(Run build, Map<String, String> record) {
this.build = build;
this.record = PackedMap.of(record);
compact();
this.record = compact(record);
}

@Deprecated
@@ -306,9 +305,8 @@ public FingerprintAction(AbstractBuild build, Map<String, String> record) {
public void add(Map<String,String> moreRecords) {
Map<String,String> r = new HashMap<String, String>(record);
r.putAll(moreRecords);
record = PackedMap.of(r);
record = compact(r);
ref = null;
compact();
}

public String getIconFileName() {
@@ -341,48 +339,20 @@ public AbstractBuild getBuild() {

@Override public void onLoad(Run<?,?> r) {
build = r;
compact();
record = compact(record);
}

@Override public void onAttached(Run<?,?> r) {
// for historical reasons this setup is done in the constructor instead
}

private void compact() {
// share data structure with nearby builds, but to keep lazy loading efficient,
// don't go back the history forever.
if (rand.nextInt(2)!=0) {
Run pb = build.getPreviousBuild();
if (pb!=null) {
FingerprintAction a = pb.getAction(FingerprintAction.class);
if (a!=null)
compact(a);
}
}
}

/**
* Reuse string instances from another {@link FingerprintAction} to reduce memory footprint.
*/
protected void compact(FingerprintAction a) {
Map<String,String> intern = new HashMap<String, String>(); // string intern map
for (Entry<String, String> e : a.record.entrySet()) {
intern.put(e.getKey(),e.getKey());
intern.put(e.getValue(),e.getValue());
}

Map<String,String> b = new HashMap<String, String>();
/** Share data structure with other builds, mainly those of the same job. */
private PackedMap<String,String> compact(Map<String,String> record) {
Map<String,String> b = new HashMap<String,String>();
for (Entry<String,String> e : record.entrySet()) {
String k = intern.get(e.getKey());
if (k==null) k = e.getKey();

String v = intern.get(e.getValue());
if (v==null) v = e.getValue();

b.put(k,v);
b.put(e.getKey().intern(), e.getValue().intern());
}

record = PackedMap.of(b);
return PackedMap.of(b);
}

/**

4 comments on commit 2f59ffc

@daniel-beck

This comment has been minimized.

Copy link
Member

@daniel-beck daniel-beck replied Dec 23, 2014

Won't this result in perm gen memory issues?

@jglick

This comment has been minimized.

Copy link
Member Author

@jglick jglick replied Dec 23, 2014

On Java 6/7 you mean (Java 8 is fine)—perhaps. I suspect you would run low on heap and start ejecting Runs well before then, which would allow Strings from the fingerprint maps to be collected.

I actually initially wrote this to keep a custom intern set but decided to go with the somewhat simpler approach of String.intern and let the JVM manage it. If you think it worthwhile I can resurrect the original code. It has the downside that pseudo-interned Strings cannot ever be collected, so on Java 8 it would be strictly worse than the current code.

@daniel-beck

This comment has been minimized.

Copy link
Member

@daniel-beck daniel-beck replied Dec 23, 2014

I don't know the details well enough to judge this. Maybe something to keep in mind when the issue actually comes up on real installs. Jenkins only requires Java 6 after all, and Java 8 isn't even in some of the default Linux package repos.

@jglick

This comment has been minimized.

Copy link
Member Author

@jglick jglick replied Dec 23, 2014

The other option is to just delete the compaction altogether (or at least on Java 7-). With the introduction of lazy-loading the intent is that most of the Runs will not actually be in memory, so the original purpose of this code becomes less relevant.

Please sign in to comment.