Skip to content
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

Modified Rfc2047's internal Token class to be a struct #951

Closed
wants to merge 2 commits into from

Conversation

jstedfast
Copy link
Owner

This reduces the number of allocations needed, but it does mean that List will be much larger.

To combat this a bit, redesigned the Token class/struct to have fewer fields which reduced sizeof(Token) from 40 bytes down to 24.

Ideally, we'd get that down even further to 16 bytes (as recommended by Microsoft). Added some comments with ideas on how to accomplish that.

This reduces the number of allocations needed, but it does mean that
List<Token> will be much larger.

To combat this a bit, redesigned the Token class/struct to have fewer
fields which reduced sizeof(Token) from 40 bytes down to 24.

Ideally, we'd get that down even further to 16 bytes (as recommended by
Microsoft). Added some comments with ideas on how to accomplish that.
@jstedfast jstedfast temporarily deployed to ci August 29, 2023 01:32 — with GitHub Actions Inactive
@jstedfast jstedfast temporarily deployed to ci August 29, 2023 01:32 — with GitHub Actions Inactive
@jstedfast jstedfast temporarily deployed to ci August 29, 2023 01:32 — with GitHub Actions Inactive
@coveralls
Copy link

Coverage Status

coverage: 0.0%. remained the same when pulling def23d9 on rfc2047-tokens into 9e2bdff on master.

Makes use of ArrayPools to minimize allocations of the internal array
as well.

This should further reduce memory allocations when decoding headers.
@jstedfast jstedfast temporarily deployed to ci August 29, 2023 02:16 — with GitHub Actions Inactive
@jstedfast jstedfast temporarily deployed to ci August 29, 2023 02:16 — with GitHub Actions Inactive
@jstedfast jstedfast temporarily deployed to ci August 29, 2023 02:16 — with GitHub Actions Inactive
@jstedfast
Copy link
Owner Author

I'm thinking that a better approach might be to have a Rfc2047TokenDecoder class/struct that is essentially a state machine that we can incrementally Push(ref Token) as we tokenize the input, thereby avoiding the need for a List or ValueList at all.

@jstedfast
Copy link
Owner Author

This idea has been replaced by #952

@jstedfast jstedfast closed this Aug 31, 2023
@jstedfast jstedfast deleted the rfc2047-tokens branch August 31, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants