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

[port] use ragel-bitmap for less memory #1624

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[port] use ragel-bitmap for less memory #1624

wants to merge 2 commits into from

Conversation

eval
Copy link
Collaborator

@eval eval commented Oct 29, 2024

port of #1623

@eval
Copy link
Collaborator Author

eval commented Oct 29, 2024

/cc @ahorek

@eval eval changed the title use ragel-bitmap for less memory [port] use ragel-bitmap for less memory Oct 29, 2024
Required removing all lib/mail/parsers/*_parser.rb and then running
`rake ragel:generate`.
@eval
Copy link
Collaborator Author

eval commented Oct 29, 2024

@ahorek it seems received_parser.rb was unchanged in the ragel-bitmap PR. This makes sense as the timestamps of the rl-files hadn't changed.
Manually deleting lib/mail/parsers/*.rb and re-running rake ragel:generate revealed it.

Could you approve this PR so it can be merged? I'll add the extra commit to 2-8-stable.
To clean up these files I created #1626 - review welcome as well 🙏🏻

@ahorek
Copy link
Contributor

ahorek commented Oct 29, 2024

Nicely spotted! Thanks for catching that.

Copy link
Contributor

@ahorek ahorek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@eval
Copy link
Collaborator Author

eval commented Oct 29, 2024

The approval needs to be from someone with commit rights, it seems... mind to help out with this ported PR @sebbASF ?

@sebbASF
Copy link
Collaborator

sebbASF commented Oct 31, 2024

Sorry, but I'm not convinced that this is a worthwhile change.
Whilst the code may be a bit smaller, it complicates the build process, and may result in stale source.

@eval
Copy link
Collaborator Author

eval commented Nov 1, 2024

@sebbASF how would you suggest to move fw?

@sebbASF
Copy link
Collaborator

sebbASF commented Nov 1, 2024

Is there really a need to change?

@ahorek
Copy link
Contributor

ahorek commented Nov 1, 2024

Compared to other dependencies, this gem has a disproportionately large memory footprint for a simple email parser. This is primarily because Ragel doesn’t generate efficient code for Ruby, and Ruby doesn’t handle large numeric arrays optimally.

#1342 cc @schneems @kddnewton

I don’t believe this complicates the build process. Modifying the Ragel sources requires recompilation regardless, and there’s already a rufo formatter in place. Additionally, the parsers were last updated in 2017, so this change shouldn’t increase maintenance costs.

@kddnewton
Copy link

I think this would be a worthwhile change, but ultimately I wouldn't be the one maintaining it so I don't think I can weigh in very heavily. I believe the memory savings are good, but if it's too complicated to understand maybe it's not worth it.

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.

4 participants