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

Update parser.rb for ruby 3.4 warning #246

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

Conversation

chaadow
Copy link

@chaadow chaadow commented Dec 25, 2024

No description provided.

Copy link
Owner

@boazsegev boazsegev left a comment

Choose a reason for hiding this comment

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

This change results in a copy of the underlying String data, whereas the original code merely changed the encoding (overwriting a few bytes in the String object data) without copying any of the bytes.

This degrades performance.

What was the warning that this commit is attempting to solve?

Thanks,
B.

@chaadow
Copy link
Author

chaadow commented Dec 26, 2024

Any force_encoding in the code must be changed somehow because force_encoding mutates the string

here i the warning:

/app/vendor/bundle/ruby/3.4.0/gems/combine_pdf-1.0.29/lib/combine_pdf/parser.rb:727: warning: string returned by :__WKANCHOR_4.to_s will be frozen in the future 

@boazsegev

@boazsegev
Copy link
Owner

Thanks!

Seems related to this:

https://github.com/ruby/ruby/blob/c6dbb10b7408cab17f170f0b23d1bbf0db03ad55/doc/NEWS/NEWS-3.4.0.md?plain=1#L139-L144

...which indicates it might relate to a specific path (or the code Encoding::ASCII_8BIT), allowing us to minimize String copying to that specific instance.

Also, __WKANCHOR_4 is a funky symbol. I think this is worth further investigation before we abandon performance considerations.

What do you think?

B.

@chaadow
Copy link
Author

chaadow commented Dec 26, 2024

Unfortunately I do not have the entire stack on sentry because i only send the first line in backtrace. If I reproduce this locally i can tell you which line led to this warning!

I've seen a lot of force_encoding inside the parser.rb file, so yeah I think a review of the entire file and ( the gem ) as well can be benefitial @boazsegev

Copy link
Author

@chaadow chaadow left a comment

Choose a reason for hiding this comment

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

@boazsegev what do you think of this instead?

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.

2 participants