-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#466: Improve performance of Pointer.dump for large dumps. #518
Conversation
… dumps. Added simle unit test for Pointer.dump.
… dumps. Added simle unit test for Pointer.dump.
StringWriter sw = new StringWriter(TITLE.length() + 2 + size * 2 + (size / BYTES_PER_ROW * 4)); | ||
PrintWriter out = new PrintWriter(sw); | ||
out.println(TITLE); | ||
// byte[] buf = getByteArray(offset, size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on discussion I can remove the commented code here and two lines below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, just curious: Were you going to remove these commented line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That goes back to the question in my original comment of this pull request, wether it's better to copy the buffer into a byte[] or to directly access the buffer. I tend to think that accessing the buffer is better, so based on that the commented lines could be removed.
I think this should be merged. Want to rebase/squash/cleanup @ck2510? |
I merged the base branch and pushed the result again. Let me know if there's anything else I can do. |
Drop the pom changes from the PR and this is good to go. |
… dumps. Pull and merge.
POM changes are gone now. |
Motivation: While we are in the channelReadComplete(...) method we might trigger flush() multiple times. This is wasteful as we will most likely do multiple sendto syscalls. We can do a better job by consolidate these flushes to just one and so be able to replace X sendto syscalls with 1 sendmmsg syscall. Modifications: - Keep track of if we are currently in the channelReadComplete(...) method and if so try to consolidate flushes. Result: Less syscalls
Also added simple unit test for Pointer.dump.
Initial implementation needed O(n) string allocations which was observed to be slow for large dumps. Using StringBuilder/StringWriter would bring down allocations to O(log n). With the implemented initial size estimate allocations should be down to O(1).
Also I tried to avoid copying the memory contents into an explicit byte[] buffer instead I'm getting the single bytes. Here it depends on the overhead of getByte() wether this is better or not. What is your opinion on that?