Seems to me that AbstractLazyLoadRunMap.all could simply create an empty BuildReference for each ID on disk, rather than calling load. Then the entrySet would have the right keys, and if and when you actually tried to access a value for an entry, BuildReferenceMapAdapter.unwrap would call getById and thus load for that one build. Right?

To answer my own question: wrong—because it is returning a map keyed by build number, which cannot be determined until records are loaded. So a fix would need to be trickier: would need to make some operations on the entry set eager (preferably ones no one is likely to call), but make .entrySet().iterator() (as used by .size() or simply anyone using a for-loop) load entries on demand in reverse numeric order.

Code changed in jenkins
User: Jesse Glick
Path:
core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
http://jenkins-ci.org/commit/jenkins/5903870d318a337548e3f7ef75603f75a378e7ac
Log:
JENKINS-18065 Documenting current misbehavior in a unit test.

Integrated in jenkins_main_trunk #2574
JENKINS-18065 Documenting current misbehavior in a unit test. (Revision 5903870d318a337548e3f7ef75603f75a378e7ac)

Result = SUCCESS
Jesse Glick : 5903870d318a337548e3f7ef75603f75a378e7ac
Files :

• core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java

Seems to block the attempted fix of JENKINS-20892 from actually working, at least in the case of BuildTimelineWidget.

The problematic part is this:

    at jenkins.model.lazy.AbstractLazyLoadRunMap.all(AbstractLazyLoadRunMap.java:617)
    at jenkins.model.lazy.AbstractLazyLoadRunMap.entrySet(AbstractLazyLoadRunMap.java:277)
    at java.util.AbstractMap$2$1.<init>(Unknown Source)
    at java.util.AbstractMap$2.iterator(Unknown Source)
    at hudson.util.RunList.iterator(RunList.java:97)

In looking at the code, what's going on is that RunList calls job.getBuilds():

    public RunList(View view) {// this is a type unsafe operation
        Set<Job> jobs = new HashSet<Job>();
        for (TopLevelItem item : view.getItems())
            jobs.addAll(item.getAllJobs());

        List<Iterable<R>> runLists = new ArrayList<Iterable<R>>();
        for (Job job : jobs) {
            runLists.add(job.getBuilds());
        }
        this.base = combine(runLists);
    }

and job.getBuilds() does this:

    public RunList<RunT> getBuilds() {
        return RunList.fromRuns(_getRuns().values());
    }

... which results in RunMap.values() which is implemented by AbstractMap, which calls ALLRM.entrySet(), that calls all().

ALLRM.entrySet() impl must be smarter, or job.getBuilds() need to avoid calling values().

Code changed in jenkins
User: Kohsuke Kawaguchi
Path:
changelog.html
core/src/main/java/hudson/util/RunList.java
core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
http://jenkins-ci.org/commit/jenkins/54c084613f83f9b7a02957eb98dbd8dc661d1c9c
Log:
[FIXED JENKINS-18065] made ALLRM.entrySet() smarter

Integrated in jenkins_main_trunk #3511
[FIXED JENKINS-18065] made ALLRM.entrySet() smarter (Revision 54c084613f83f9b7a02957eb98dbd8dc661d1c9c)

Result = SUCCESS
kohsuke : 54c084613f83f9b7a02957eb98dbd8dc661d1c9c
Files :

• core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
• core/src/main/java/hudson/util/RunList.java
• changelog.html
• core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
• core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java

Code changed in jenkins
User: Jesse Glick
Path:
core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
http://jenkins-ci.org/commit/jenkins/da57ad5c30b1c9ca8365f869638abd29c8fb5896
Log:
JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal.

Compare: https://github.com/jenkinsci/jenkins/compare/54c084613f83...da57ad5c30b1

Code changed in jenkins
User: Jesse Glick
Path:
cli/pom.xml
core/pom.xml
plugins/pom.xml
pom.xml
test/pom.xml
war/pom.xml
http://jenkins-ci.org/commit/jenkins/0faad29a5594fc20f9b3670797a3b7ebb198b63b
Log:
Set version to: 1.554.3.JENKINS-18065-ALLRM-all

Code changed in jenkins
User: Kohsuke Kawaguchi
Path:
core/src/main/java/hudson/util/RunList.java
core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
http://jenkins-ci.org/commit/jenkins/b227250a63beab888a25c2a781b6fe8e493284d3
Log:
[FIXED JENKINS-18065] made ALLRM.entrySet() smarter

(cherry picked from commit 54c084613f83f9b7a02957eb98dbd8dc661d1c9c)

Conflicts:
changelog.html

Code changed in jenkins
User: Jesse Glick
Path:
core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
http://jenkins-ci.org/commit/jenkins/525793d741c5f209b27bdc94844798b6410fce7b
Log:
JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal.
(cherry picked from commit da57ad5c30b1c9ca8365f869638abd29c8fb5896)

Code changed in jenkins
User: Jesse Glick
Path:
core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
core/src/test/java/jenkins/model/lazy/FakeMap.java
http://jenkins-ci.org/commit/jenkins/c3e73296b5af0a4aaa06a3bdf867ac50fe211888
Log:
JENKINS-18065 Uncommenting original test.
The actual implementation does not behave as nicely as one would hope, for a few reasons:
1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk.
Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty.
2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries.
You would think that it would suffice to check for the last member of idOnDisk in index.byId.
3. The iterator eagerly loads the next value before hasNext has even been called.
Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway.
Might cause one extra build record to be loaded unnecessarily from a limited RunList.

(cherry picked from commit 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)

Integrated in jenkins_main_trunk #3512
JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal. (Revision da57ad5c30b1c9ca8365f869638abd29c8fb5896)

Result = SUCCESS
Jesse Glick : da57ad5c30b1c9ca8365f869638abd29c8fb5896
Files :

• core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java

Code changed in jenkins
User: Jesse Glick
Path:
core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
core/src/test/java/jenkins/model/lazy/FakeMap.java
http://jenkins-ci.org/commit/jenkins/2f58ceb1a66c8ed621fa2562c5ed3445b29ea200
Log:
JENKINS-18065 Uncommenting original test.
The actual implementation does not behave as nicely as one would hope, for a few reasons:
1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk.
Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty.
2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries.
You would think that it would suffice to check for the last member of idOnDisk in index.byId.
3. The iterator eagerly loads the next value before hasNext has even been called.
Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway.
Might cause one extra build record to be loaded unnecessarily from a limited RunList.

Deployed https://github.com/jenkinsci/jenkins/compare/jenkins-1.554.3...JENKINS-18065-ALLRM-all to: http://repo.jenkins-ci.org/public/org/jenkins-ci/main/jenkins-war/1.554.3.JENKINS-18065-ALLRM-all/jenkins-war-1.554.3.JENKINS-18065-ALLRM-all.war

Integrated in jenkins_main_trunk #3514
JENKINS-18065 Uncommenting original test. (Revision 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)

Result = SUCCESS
Jesse Glick : 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200
Files :

• core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
• core/src/test/java/jenkins/model/lazy/FakeMap.java

Code changed in jenkins
User: Daniel Beck
Path:
changelog.html
http://jenkins-ci.org/commit/jenkins/fdc0b5c5650b3ee849f02e3c5d94d23c12886adc
Log:
Noting #1314, #1316, #1308, JENKINS-17667, JENKINS-22395, JENKINS-18065

Code changed in jenkins
User: Daniel Beck
Path:
changelog.html
http://jenkins-ci.org/commit/jenkins/72f37e414533c02624f9deffc85a2b50f03905e6
Log:
Remove JENKINS-22395, JENKINS-18065 from 1.572 section

Code changed in jenkins
User: Kohsuke Kawaguchi
Path:
core/src/main/java/hudson/util/RunList.java
core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
http://jenkins-ci.org/commit/jenkins/632d7fe0d01d492e0803495691f4397980cf44e0
Log:
[FIXED JENKINS-18065] made ALLRM.entrySet() smarter

(cherry picked from commit 54c084613f83f9b7a02957eb98dbd8dc661d1c9c)

Conflicts:
changelog.html

Code changed in jenkins
User: Jesse Glick
Path:
core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
http://jenkins-ci.org/commit/jenkins/f1b0779c6d356db8fb0de6c89bd5bc03483d6b34
Log:
JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal.
(cherry picked from commit da57ad5c30b1c9ca8365f869638abd29c8fb5896)

Code changed in jenkins
User: Jesse Glick
Path:
core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
core/src/test/java/jenkins/model/lazy/FakeMap.java
http://jenkins-ci.org/commit/jenkins/8c70a2b76875d2ca6a114c45fd54c0b1e9ecb7d2
Log:
JENKINS-18065 Uncommenting original test.
The actual implementation does not behave as nicely as one would hope, for a few reasons:
1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk.
Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty.
2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries.
You would think that it would suffice to check for the last member of idOnDisk in index.byId.
3. The iterator eagerly loads the next value before hasNext has even been called.
Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway.
Might cause one extra build record to be loaded unnecessarily from a limited RunList.

(cherry picked from commit 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)