[FIXED JENKINS-39535] - Optimize get log method #2607
Conversation
Jimilian
force-pushed
the
optimize_getLog_method
branch
2 times, most recently
from
9c11d13
to
e15852a
Compare
It should speed up and reduce memory consumption for some plugins (i.e. Email-ext Plugin). Also now this method could be used to get last lines of build output in efficient manner.
Jimilian
force-pushed
the
optimize_getLog_method
branch
from
e15852a
to
fa6ef08
Compare
oleg-nenashev
added
the
needs-more-reviews
| import java.io.Reader; | ||
| import java.io.StringWriter; | ||
|
|
||
| import java.io.*; |
There was a problem hiding this comment.
Please do not do it in the production code. We have so many internal classes with "common" names and interfaces. It may cause implicit ambiguous behavior
| } | ||
|
|
||
| int lines = 0; | ||
| final List<Byte> bytes = new ArrayList<>(); |
There was a problem hiding this comment.
Initialize by something reasonably small? E.g. 256. Just a performance hack
There was a problem hiding this comment.
bytes.clear() doesn't resize array, it means that capacity of array would grow until reaching max line length. And after that capacity would be constant. So, I don't see profit in this hack.
| // operations. | ||
| if (lineCount > maxLines) | ||
| logLines.remove(0); | ||
| final List<String> lastLines = new ArrayList<>(maxLines); |
There was a problem hiding this comment.
🐛 if somebody is dare enough to pass MAX_INT in his code, it will consume too much memory. I would limit it by something reasonably small (e.g. 1024) if you really need an ArrayList
| if (lineCount > maxLines) | ||
| logLines.set(0, "[...truncated " + (lineCount - (maxLines - 1)) + " lines...]"); | ||
| if (lines == maxLines) { | ||
| lastLines.set(0, "[...truncated lines...]"); |
There was a problem hiding this comment.
🐜 no info about number of truncated lines left. I understand it's not possible in the current implementation, but maybe "[....truncated N bytes..]." could work as a replacement
|
|
||
| return ConsoleNote.removeNotes(logLines); | ||
| private String convertBytesToString(List<Byte> bytes) { | ||
| Collections.reverse(bytes); |
There was a problem hiding this comment.
IIRC there is a method, which creates new collection from ArrayList or LinkedList without updating the original one. Should be better from the performance PoV. Or you can just use Reverese iterator since you have an Array list
There was a problem hiding this comment.
Are you talking about converting bytes to String? or reverse()?
reverse doesn't create new collection, doesn't allocate it, etc. Just pure swap. I can't imagine anything faster.
Anyway collections and allocations in this method should be quite small.
| private String convertBytesToString(List<Byte> bytes) { | ||
| Collections.reverse(bytes); | ||
| Byte[] byteArray = bytes.toArray(new Byte[bytes.size()]); | ||
| return new String(ArrayUtils.toPrimitive(byteArray), Charset.forName("UTF-8")); |
There was a problem hiding this comment.
🐛 Original code was using getCharset()
|
@oleg-nenashev Looks better now? |
| @@ -189,7 +189,7 @@ public void getLogReturnsAnRightOrder() throws Exception { | |||
| for (int i = 1; i < 10; i++) { | |||
| assertEquals("dummy" + (10+i), logLines.get(i)); | |||
| } | |||
| assertEquals("[...truncated lines...]", logLines.get(0)); | |||
| assertEquals("[...truncated 68 B...]", logLines.get(0)); | |||
There was a problem hiding this comment.
Not perfect, but this is a standard method
|
LGTM 👍 |
|
Does this affect the gzipped log file use case? |
|
@daniel-beck good catch! It would not work for gzipped files. So, I can check file type and switch to old implementation if file is gzipped. Because AFAIK it's impossible to read gzipped file from end. |
Is there test case for it? |
|
@KostyaSha No, I found only one test case for this method in core - that method returns empty list instead of |
|
So where gzipped logs at all exists? |
|
@daniel-beck how this old implementation may use gzipped log files? Method directly opens file and reads lines, why it should affect gzipped logs? |
|
@KostyaSha Not sure, hence me asking about it. If there's no impact (e.g. because this code is never called on a finalized log file), that's great. But a number of other methods of |
|
I do not believe it impacts gzipped logs since the change operates within existing public method. GZIP build logging should have a completely different implementation/override. One of IMPLs is here: https://wiki.jenkins-ci.org/display/JENKINS/Compress+Build+Log+Plugin. If the method is broken for this case, it is unlikely a regression |
Maintainer(s): Daniel Beck |
|
@KostyaSha @oleg-nenashev I have no idea what you are referring to. The core feature handling compressed build logs – the concern here – predates the sample-code sized plugin that automatically compresses them by half a decade: d300ff8 |
|
@daniel-beck But previous implementation of |
|
@daniel-beck @KostyaSha WDYT? |
oleg-nenashev
added
the
ready-for-merge
|
As @Jimilian points out, the previous version had no gzip support either. So that part is okay. Beyond that, no opinion, as I haven't had a chance to look into this further. |
oleg-nenashev
changed the title
|
Merging since we have 2 +1s. I doubt it's a subject for backporting, but I've created JENKINS-39535 just in case |
|
Did this cause https://issues.jenkins-ci.org/browse/JENKINS-39555 ? |
|
@daniel-beck hard to see how; Stage View uses Pipeline-specific log storage, not methods on |
Only for tests ( |
|
@jglick it was used by some plugins. In our case it was plugin that sends mail notifications in case of failed build. |
Current implementation of
getLog(int maxLines)reads entire log file.In this PR I'm addressing this issue. Instead of reading all lines from start, I'm readying only maxLines from end of log.