Fix recursion

[artem-fedorov]

No description provided.

[codecov]

Codecov Report

Merging #179 into master will increase coverage by 0.03%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master     #179      +/-   ##
==========================================
+ Coverage   71.83%   71.86%   +0.03%     
==========================================
  Files          45       45              
  Lines        4204     4205       +1     
  Branches      743      743              
==========================================
+ Hits         3020     3022       +2     
  Misses        892      892              
+ Partials      292      291       -1

Impacted Files	Coverage Δ
...ns/performance/actions/PerformanceBuildAction.java	78.78% <80%> (+0.66%)	⬆️
...dson/plugins/performance/PerformanceReportMap.java	71.96% <92.85%> (+0.37%)	⬆️
.../hudson/plugins/performance/reports/UriReport.java	72.14% <0%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bcf4ea...d8c7b9b. Read the comment docs.

[tilln]

Hi @artem-fedorov

Would you mind explaining why this loadPreviousBuilds() needed to be added?
It wasn't there before, and I can't figure it out from just looking at the code.

Reason I'm asking is that since upgrading the plugin version we're seeing some performance impact with threads spending literally thousands of seconds parsing (see below stack trace fragment from thread dump), presumably going through the entire job history.

[...]
hudson.plugins.performance.parsers.AbstractParser.initDateFormat(AbstractParser.java:225)
hudson.plugins.performance.parsers.AbstractParser.parseTimestamp(AbstractParser.java:207)
hudson.plugins.performance.parsers.JMeterCsvParser.getSample(JMeterCsvParser.java:136)
hudson.plugins.performance.parsers.JMeterCsvParser.parseCSV(JMeterCsvParser.java:80)
hudson.plugins.performance.parsers.JMeterCsvParser.parse(JMeterCsvParser.java:70)
hudson.plugins.performance.parsers.AbstractParser.parse(AbstractParser.java:85)
hudson.plugins.performance.PerformanceReportMap.parseReports(PerformanceReportMap.java:491)
hudson.plugins.performance.PerformanceReportMap.<init>(PerformanceReportMap.java:78)
hudson.plugins.performance.actions.PerformanceBuildAction.getPerformanceReportMap(PerformanceBuildAction.java:87)
hudson.plugins.performance.PerformanceReportMap.getReportMap(PerformanceReportMap.java:534)
hudson.plugins.performance.PerformanceReportMap.loadPreviousBuilds(PerformanceReportMap.java:506)
hudson.plugins.performance.PerformanceReportMap.<init>(PerformanceReportMap.java:89)
hudson.plugins.performance.actions.PerformanceBuildAction.getPerformanceReportMap(PerformanceBuildAction.java:87)
hudson.plugins.performance.actions.PerformanceBuildAction.getTarget(PerformanceBuildAction.java:62)
hudson.plugins.performance.actions.PerformanceBuildAction.getTarget(PerformanceBuildAction.java:18)
[...]

[artem-fedorov]

@tilln
Hi. I can't be sure now, but we need load all reports for create Trend history

[tilln]

Well, it seems to work with that line removed though.

[artem-fedorov]

did you test it after restart Jenkins server or open other job?

[tilln]

Definitely after restart (needed for plugin redeployment).

[tilln]

Worth noting is also that serialized report files seem to get parsed over and over again, i.e. thread dumps show JMeterCsvParser.parse instead of AbstractParser.loadSerializedReport. No idea what would have caused that to change.
Edit: That's seems to be a side effect of loadPreviousBuilds() going through all builds, see below.

[tilln]

@artem-fedorov
I have done some more digging and the performance impact seems to be caused by 2 compounding things (in conjunction with a significant build history and not small CSV files):

1. A bug in AbstractParser.loadSerializedReport that makes the report caching ineffective and leads to parsing for previously cached reports.
2. The "new" loadPreviousBuilds() loads more than previously, frequently hitting the cache (leading to reparsing).
   Also, it appears to be redundant as addPreviousBuildReports() loads what is needed.

I'll have submitted PRs #188 and #189 so they can be reviewed individually.