Fix bad scaling in rbs_comment_t tokens
#2578
Merged
Conversation
This file contains hidden or 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
amomchilov
commented
Jun 27, 2025
Comment on lines
-3202
to
-3203
| com->tokens = rbs_allocator_calloc(allocator, com->line_size, rbs_token_t); | ||
| memcpy(com->tokens, p, sizeof(rbs_token_t) * com->line_count); |
There was a problem hiding this comment.
This was just re-implementing realloc
src/parser.c
Outdated
| @@ -3213,19 +3211,22 @@ static void comment_insert_new_line(rbs_allocator_t *allocator, rbs_comment_t *c | |||
| static rbs_comment_t *alloc_comment(rbs_allocator_t *allocator, rbs_token_t comment_token, rbs_comment_t *last_comment) { | |||
| rbs_comment_t *new_comment = rbs_allocator_alloc(allocator, rbs_comment_t); | |||
|
|
|||
| size_t initial_line_size = 10; | |||
There was a problem hiding this comment.
This is the same initial size this code used to use before. I haven't spent any time tuning it.
8c30976 to
1dbb362
Compare
1dbb362 to
c0c8021
Compare
amomchilov
commented
Jul 2, 2025
Comment on lines
+30
to
+32
| size_t line_tokens_capacity; | ||
| size_t line_tokens_count; | ||
| rbs_token_t *line_tokens; |
There was a problem hiding this comment.
This naming was confusing, because line_size sounds like "the size of a line".
What is actually means is "the capacity of the line_tokens buffer.
amomchilov
commented
Jul 2, 2025
| @@ -3160,42 +3160,43 @@ static rbs_comment_t *comment_get_comment(rbs_comment_t *com, int line) { | |||
| } | |||
|
|
|||
| static void comment_insert_new_line(rbs_allocator_t *allocator, rbs_comment_t *com, rbs_token_t comment_token) { | |||
| if (com->line_count == 0) { | |||
There was a problem hiding this comment.
By changing alloc_comment() to not call comment_insert_new_line(), we never have to deal with the 0 case here 🥳
|
@amomchilov Can you take a look at the clang-format failure? Simply rebasing the branch on the top of master would solve the issue. |
c0c8021 to
be715d4
Compare
|
@soutaro Done! |
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.
Suggestion cannot be applied right now. Please check back later.
line_sizewas growing arithmetically (+ 10) rather than geometrically (* 2). I chose* 2because it's a popular choice and matchesrbs_buffer_t.Here's some stats on the number of lines per comment
You can see a huge left skew:
77% of comments are 20 lines or less:
Allocation stats
Before
These tables show how many comments required a particular number of reallocations.
After
By some rough numbers, I think this changes reduces needless memory copying from 5.79TB to only 0.89 TB.