Skip to content

[JENKINS-75986] Refactor BuildReferenceMapAdapter #10946

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.

Sign up for GitHub

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

Merged
timja merged 9 commits into from
Aug 20, 2025
Merged

[JENKINS-75986] Refactor BuildReferenceMapAdapter #10946

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
48768b1
JENKINS-75986: Refactor BuildReferenceMapAdapter to improve code re-u…
Aug 10, 2025
2d8019b
Update core/src/main/java/jenkins/model/lazy/BuildReferenceMapAdapter…
dukhlov Aug 15, 2025
e36ec32
Update core/src/main/java/jenkins/model/lazy/BuildReferenceMapAdapter…
dukhlov Aug 15, 2025
ac7f883
Fix reviewed finding
Aug 15, 2025
b37f13c
Fix formatting
dukhlov Aug 16, 2025
5a44930
Fix tests
dukhlov Aug 16, 2025
39e329d
Fix tests 2.
dukhlov Aug 16, 2025
5fff544
Merge branch 'master' into refacrot-lazy-loading-part2
dukhlov Aug 16, 2025
955a8ed
Merge branch 'master' into refacrot-lazy-loading-part2
dukhlov Aug 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/model/RunMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ protected String getIdOf(R r) {

/**
* Add a <em>new</em> build to the map.
* Do not use when loading existing builds (use {@link #put(Integer, Object)}).
* Do not use when loading existing builds (use {@link #putAll(Map)}).
Copy link
Member

@jglick jglick Aug 15, 2025

Choose a reason for hiding this comment

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

Left over from the LazyBuildMixIn change in #10759 I guess.

Copy link
Contributor Author

@dukhlov dukhlov Aug 15, 2025

Choose a reason for hiding this comment

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

right

*/
@Override
public R put(R r) {
Expand Down
211 changes: 32 additions & 179 deletions core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,7 @@
import hudson.util.CopyOnWriteMap;
import java.io.File;
import java.io.IOException;
import java.util.AbstractCollection;
import java.util.AbstractMap;
import java.util.AbstractSet;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
Expand All @@ -46,10 +44,7 @@
import java.util.NoSuchElementException;
import java.util.Set;
import java.util.SortedMap;
import java.util.Spliterator;
import java.util.Spliterators;
import java.util.TreeMap;
import java.util.function.IntConsumer;
import java.util.function.IntPredicate;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand Down Expand Up @@ -101,146 +96,28 @@
public abstract class AbstractLazyLoadRunMap<R> extends AbstractMap<Integer, R> implements SortedMap<Integer, R> {
private final CopyOnWriteMap.Tree<Integer, BuildReference<R>> core = new CopyOnWriteMap.Tree<>(
Collections.reverseOrder());

private LazyLoadRunMapEntrySet<R> entrySet = new LazyLoadRunMapEntrySet<>(this);

private transient volatile Set<Integer> keySet;
private transient volatile Collection<R> values;
private final BuildReferenceMapAdapter<R> adapter = new BuildReferenceMapAdapter<>(core, this::resolveBuildRef,
this::getNumberOf) {
@Override
protected boolean removeValue(R value) {
return AbstractLazyLoadRunMap.this.removeValue(value);

Check warning on line 103 in core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 103 is not covered by tests
}
};

@Override
public Set<Integer> keySet() {
Set<Integer> ks = keySet;
if (ks == null) {
ks = new AbstractSet<>() {
@Override
public Iterator<Integer> iterator() {
return new Iterator() {
private final Iterator<Entry<Integer, R>> it = entrySet().iterator();

@Override
public boolean hasNext() {
return it.hasNext();
}

@Override
public Integer next() {
return it.next().getKey();
}

@Override
public void remove() {
it.remove();
}
};
}

@Override
public Spliterator<Integer> spliterator() {
return new Spliterators.AbstractIntSpliterator(
Long.MAX_VALUE,
Spliterator.DISTINCT | Spliterator.ORDERED | Spliterator.SORTED) {
private final Iterator<Integer> it = iterator();

@Override
public boolean tryAdvance(IntConsumer action) {
if (action == null) {
throw new NullPointerException();
}
if (it.hasNext()) {
action.accept(it.next());
return true;
}
return false;
}

@Override
public Comparator<Integer> getComparator() {
return Collections.reverseOrder();
}
};
}

@Override
public int size() {
return AbstractLazyLoadRunMap.this.size();
}

@Override
public boolean isEmpty() {
return AbstractLazyLoadRunMap.this.isEmpty();
}

@Override
public void clear() {
AbstractLazyLoadRunMap.this.clear();
}

@Override
public boolean contains(Object k) {
return AbstractLazyLoadRunMap.this.containsKey(k);
}
};
keySet = ks;
}
return ks;
return adapter.keySet();
}

@Override
public Collection<R> values() {
Collection<R> vals = values;
if (vals == null) {
vals = new AbstractCollection<>() {
@Override
public Iterator<R> iterator() {
return new Iterator<>() {
private final Iterator<Entry<Integer, R>> it = entrySet().iterator();

@Override
public boolean hasNext() {
return it.hasNext();
}

@Override
public R next() {
return it.next().getValue();
}

@Override
public void remove() {
it.remove();
}
};
}

@Override
public Spliterator<R> spliterator() {
return Spliterators.spliteratorUnknownSize(
iterator(), Spliterator.DISTINCT | Spliterator.ORDERED);
}

@Override
public int size() {
return AbstractLazyLoadRunMap.this.size();
}

@Override
public boolean isEmpty() {
return AbstractLazyLoadRunMap.this.isEmpty();
}

@Override
public void clear() {
AbstractLazyLoadRunMap.this.clear();
}
return adapter.values();
}

@Override
public boolean contains(Object v) {
return AbstractLazyLoadRunMap.this.containsValue(v);
}
};
values = vals;
}
return vals;
@Override
public Set<Entry<Integer, R>> entrySet() {
assert baseDirInitialized();

Check warning on line 119 in core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 119 is only partially covered, one branch is missing
return adapter.entrySet();
}

/**
Expand Down Expand Up @@ -354,18 +231,22 @@

@Override
public Comparator<? super Integer> comparator() {
return Collections.reverseOrder();
return core.comparator();

Check warning on line 234 in core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 234 is not covered by tests
}

@Override
public boolean isEmpty() {
return search(Integer.MAX_VALUE, DESC) == null;
return adapter.isEmpty();
}

@Override
public Set<Entry<Integer, R>> entrySet() {
assert baseDirInitialized();
return entrySet;
public boolean containsKey(Object value) {
return adapter.containsKey(value);
}

@Override
public boolean containsValue(Object value) {
return adapter.containsValue(value);

Check warning on line 249 in core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 244-249 are not covered by tests
}

/**
Expand All @@ -379,7 +260,7 @@
res.put(entry.getKey(), buildRef);
}
}
return Collections.unmodifiableSortedMap(new BuildReferenceMapAdapter<>(this, res));
return Collections.unmodifiableSortedMap(new BuildReferenceMapAdapter<>(res, this::resolveBuildRef, this::getNumberOf));
}

/**
Expand All @@ -390,33 +271,17 @@
*/
@Override
public SortedMap<Integer, R> subMap(Integer fromKey, Integer toKey) {
// TODO: if this method can produce a lazy map, that'd be wonderful
// because due to the lack of floor/ceil/higher/lower kind of methods
// to look up keys in SortedMap, various places of Jenkins rely on
// subMap+firstKey/lastKey combo.

R start = search(fromKey, DESC);
if (start == null) return EMPTY_SORTED_MAP;

R end = search(toKey, ASC);
if (end == null) return EMPTY_SORTED_MAP;

for (R i = start; i != end; ) {
i = search(getNumberOf(i) - 1, DESC);
assert i != null;
}
Comment on lines -404 to -407
Copy link
Member

@jglick jglick Aug 15, 2025

Choose a reason for hiding this comment

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

Unclear to me what this loop was even doing (when assertions are disabled).

Copy link
Contributor Author

@dukhlov dukhlov Aug 15, 2025

Choose a reason for hiding this comment

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

This code should be removed in scope of #10759. As far as I understood, this loop was loading builds which belong to subMap interval to Index.

Copy link
Contributor Author

@dukhlov dukhlov Aug 15, 2025

Choose a reason for hiding this comment

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

Now AbstractLazyLoadRunMap.core (which replaces Index) has all set of build references available on disk by design


return Collections.unmodifiableSortedMap(new BuildReferenceMapAdapter<>(this, core.subMap(fromKey, toKey)));
return adapter.subMap(fromKey, toKey);
}

@Override
public SortedMap<Integer, R> headMap(Integer toKey) {
return subMap(Integer.MAX_VALUE, toKey);
return adapter.headMap(toKey);
}

@Override
public SortedMap<Integer, R> tailMap(Integer fromKey) {
return subMap(fromKey, Integer.MIN_VALUE);
return adapter.tailMap(fromKey);

Check warning on line 284 in core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 284 is not covered by tests
}

@Override
Expand All @@ -443,11 +308,7 @@

@Override
public R get(Object key) {
if (key instanceof Integer) {
int n = (Integer) key;
return get(n);
}
return super.get(key);
return adapter.get(key);
}

public R get(int n) {
Expand Down Expand Up @@ -552,7 +413,7 @@
}

public R getByNumber(int n) {
return resolveBuildRef(core.get(n));
return adapter.get(n);
}

/**
Expand Down Expand Up @@ -597,14 +458,9 @@
core.putAll(newWrapperData);
}

/**
* Return underlining {@link BuildReference} core map.
*
* @return
* full build reference map.
*/
/*package*/ SortedMap<Integer, BuildReference<R>> all() {
return core;
@Override
public R remove(Object key) {
return adapter.remove(key);

Check warning on line 463 in core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 463 is not covered by tests
}

/**
Expand Down Expand Up @@ -652,7 +508,6 @@
return new BuildReference<>(getIdOf(r), r);
}


/**
* Parses {@code R} instance from data in the specified directory.
*
Expand Down Expand Up @@ -694,7 +549,5 @@
ASC, DESC, EXACT
}

private static final SortedMap EMPTY_SORTED_MAP = Collections.unmodifiableSortedMap(new TreeMap());

static final Logger LOGGER = Logger.getLogger(AbstractLazyLoadRunMap.class.getName());
}
Loading
Loading