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

Lazy parsing #143

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

Lazy parsing #143

wants to merge 8 commits into from

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Aug 2, 2023

Currently an instance can only pass one property, so in this case the latter assertion will fail.

fi = FastImage.new(File.join(FixturePath, "animated.gif"))
assert_equal [400, 400], fi.size
assert_equal true, fi.animated

This PR makes adds lazy parsing. With the new lazy option, parsing isn't initiated from the constructor, and new properties are parsed on demand.

In particular, File.size? is now only invoked if content_length is requested. This is a small performance improvement.

Instead of returning the result, the different parsers now populate @size, @orientation etc. whenever it encounters the corresponding data. This allows a parser to populate more than one property in one go.

For backwards compatibility, lazy defaults to false. In a future major release, this could change to true, and type_only/size_only/animated_only can be removed.

lib/fastimage.rb Outdated
end

def height
size && size.second
Copy link
Owner

Choose a reason for hiding this comment

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

Note that Array#second doesn't exist outside of active support, have to use size[1]

def fetch_using_file_open
@content_length = File.size?(@uri)
def fetch_using_file_open(property)
return @content_length = File.size?(@uri) if property == :content_length
Copy link
Owner

Choose a reason for hiding this comment

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

It would seem a bit neater to me to always set @content_length here - and after that do the early return if content length was being asked for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentional.

It was actually what inspired me to make this PR. I had noticed that Fastimage is a bit slower (~10-20% AFAIR) than the GIF reader in Mastodon. I found out that the difference is due to the File.size? call in Fastimage, so it makes sense to skip this unless content_length is explicitly requested.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah ok!

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