Unexpected memory footprint from LogRotator

[Jimilian]

Hi!

We had some issues with Jenkins last days and after some profiling I found that root cause is how Jenkins perform log rotation.

LogRotator.java (1.609.3):

        if(numToKeep!=-1) {
            // Note that RunList.size is deprecated, and indeed here we are loading all the builds of the job.
            // However we would need to load the first numToKeep anyway, just to skip over them;
            // and we would need to load the rest anyway, to delete them.
            // (Using RunMap.headMap would not suffice, since we do not know if some recent builds have been deleted for other reasons,
            // so simply subtracting numToKeep from the currently last build number might cause us to delete too many.)
            List<? extends Run<?,?>> builds = job.getBuilds();
            for (Run r : copy(builds.subList(Math.min(builds.size(), numToKeep), builds.size()))) {
                if (shouldKeepRun(r, lsb, lstb)) {
                    continue;
                }
                LOGGER.log(FINE, "{0} is to be removed", r);
                r.delete();
            }
        }

I think it's much better to keep "numToKeep - X" or "numToKeep + X" jobs instead of loading and processing all builds.
Because now it creates a huge... HUGE... memory footprint:


Maybe it's possible to change this behavior? Create a additional option or something like that? Because right now we have only one solution - use date based rotation instead of number of builds based.

Br, Alex

[Jimilian]

[Jesse Glick]

On Mon, Jun 13, 2016 at 10:49 AM, Jimilian <mr.ak...@gmail.com> wrote:
> Maybe it's possible to change this behavior?

Reread the comment you quoted.

[Jimilian]

I had read it very careful. And want to repeat my point - I think it's much better to delete a little bit more jobs (or less) than to pay so huge price (25% of all allocations!!! see screenshot in my second message).

If I didn't get something from this comment, please, clarify it for me.

[Jesse Glick]

On Mon, Jun 13, 2016 at 12:27 PM, Jimilian <mr.ak...@gmail.com> wrote:
> I think it's much
> better to delete a little bit more jobs (or less) than to pay so huge price

Then we would need to introduce new UI to allow the user to decide to
delete all builds older than a certain number.

Post-1.597 it may be possible to introduce new APIs into `RunList`
allowing cheap inspection of the _existence_ of a build with a given
number, which would allow the current semantics to be maintained more
cheaply.

[Jimilian]

Maybe somebody will find it useful:

https://wiki.jenkins-ci.org/display/JENKINS/Build+Rotator+Plugin

This implementation just doesn't care about accurate number of jobs :)

If user specified 100, history will contain NO MORE than 100 builds.

Br, Alex

[Jesse Glick]

On Thu, Feb 23, 2017 at 4:39 PM, Jimilian <mr.ak...@gmail.com> wrote:
> https://wiki.jenkins-ci.org/display/JENKINS/Build+Rotator+Plugin
>
> This implementation just doesn't care about accurate number of jobs

Better to fix this in core by enhancing `RunList`.