Cache resolved data in Manage Old Data page#26361
Cache resolved data in Manage Old Data page#26361subwaycookiecrunch wants to merge 1 commit intojenkinsci:masterfrom
Conversation
|
@subwaycookiecrunch The description says "Fixes #16843" but the title references JENKINS-76278. Which issue does this actually fix? Please align the "Fixes" line with the correct issue. |
| <j:if test="${item.value.extra!=null and item.value==''}"> | ||
| <j:set var="count" value="${0}"/> | ||
| <j:forEach var="item" items="${allData.entrySet()}"> | ||
| <j:if test="${item.value.extra!=null and item.value=='' and count < maxDisplay}"> |
There was a problem hiding this comment.
why do you need to encode this? you use a > elsewhere on the page?
| <j:if test="${item.value.extra!=null and item.value=='' and count < maxDisplay}"> | |
| <j:if test="${item.value.extra!=null and item.value=='' and count < maxDisplay}"> |
There was a problem hiding this comment.
@timja, I think it might be because in XML, inside an attribute value, < is usually escaped as < so the parser doesn’t treat it as the start of a tag, while > typically doesn’t need escaping. So the < here might be intentional, and using > elsewhere on the page doesn’t necessarily contradict that. I could be wrong; if in Jelly it’s safe to use a raw < in the attribute, then simplifying makes sense.
There was a problem hiding this comment.
I'd try it and if it doesn't work its fine but better to try first
There was a problem hiding this comment.
@timja @andreahlert right, raw < does break the XML parser in attribute values so that won't work. I just flipped the comparison to maxDisplay > count instead , no entity needed and it's consistent with how I already wrote the check on line 52. Pushed.
948133c to
cdd68c6
Compare
|
@andreahlert yeah sorry about that, #16843 is the GitHub mirror of JENKINS-76278 , they're the same issue. I'll clean up the description so it's not confusing. Thanks for catching it. |
Fixes #16843 (JENKINS-76278)
manage.jellycallsit.datathree times — once to scan flags, once for the version table, once for the unreadable data table.getData()iterates every entry and resolves eachRunSaveableReferencethroughRun.fromExternalizableId(), which does a job lookup + build-by-number lookup. With 30k+ entries that's roughly 90k lookups for a single page load, enough to peg the CPU for 40+ minutes.Stashed the result in a Jelly variable so resolution happens once. Also capped the table at 1000 rows since rendering 30k
<tr>elements locks up the browser regardless of backend speed. The cap is display-only — upgrade and discard still work on the full data set.Testing done
Added
managePageRendering()toOldDataMonitorTest— registers old data on a build, confirms it shows up ingetData(), then loads the manage page viaWebClientand verifies it renders without error.Existing tests (memory, slowDiscard, unlocatableRun) continue to pass since the Java side is unchanged.
Screenshots (UI changes only)
N/A, no visual design changes, just a display cap with a warning message when entry count exceeds 1000.
Before
After
Proposed changelog entries
Proposed changelog category
/label bug
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@basil @MarkEWaite