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
Tweaks to reduce memory use of short commands #5837
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This avoids creation of most Charset encoders, decoders, buffers, and some intermediate byte[] and char[] instances created at boot and by the parser.
The yacc logic did not use the out variable. The ripper logic was simple to change to use a field. This eliminates allocation of thousands of Encoding[1] during gem list.
Many arrays in the system are single elements, both in user code and within our AST to represent things like argument lists. This logic reduces the amount of wasted Node[] instances when a given ListNode will only ever contain one element. If a ListNode with a single element is grown, this logic returns to a Node[] from then on. This change, tested against `--dev -S gem list` reduce Node[] use by the following numbers: Before: * Node[] allocated: 48002 (1536064 bytes) * Node[] retained: 24900 (796800 bytes) After: * Node[] allocated: 12390 (612576 bytes) * Node[] retained: 6142 (303704 bytes)
The former pattern caused a StringBuilder and accompanying buffer to be instantiated just to append a character and turn the buffer into a String, at a cost of another additional char[]. String.valueOf allocates only the one-element char[] and the String object itself.
I have re-pushed this with the failing source position patch moved to #5838. |
kares
reviewed
Aug 18, 2019
kares
approved these changes
Aug 18, 2019
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 one Thread.dumpStack()
left-over but otherwise looks reasonable to me
This was inserted to detect if this Iterator implementation were ever used. None of my tests produced any output, and it apparently did not interfere with any tests in CI. Maybe it was needed before but it is apparently not used now.
I'm going to go ahead and merge this. @enebo any review issues you find I will patch later. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
This is a set of memory/alloc-reducing experiments that can be merged or cherry-picked as appropriate.