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 load image content #137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Lazy load image content #137

wants to merge 1 commit into from

Conversation

eneagoe
Copy link

@eneagoe eneagoe commented Jul 15, 2019

Instead of reading the image content from files/objects at declaration, use a lambda to delay reading the image data only when the image is actually inserted in the template.

The main reason for this would be to save some memory: we have a scenario when we need to make a lot of images available to a template renderer, but the templates are dynamically generated and we don't know upfront which images (if any) will be used by the templates.

  Instead of reading the image content from files/objects at
  declaration, use a lambda to delay reading the image data only when
  the image is actually inserted in the template.
@stadelmanma
Copy link
Collaborator

Hi @eneagoe I think this is a good idea, but the one challenge I can think of it the opposite case. How would this affect performance if you only had a few images that needed reused hundreds, thousands etc. of times. Also, I haven't tested it but it looks like this code might break if an image is used twice in the same template, even if it doesn't you are re-reading the image every time it's used.

There is also a degree of added complexity and, while I don't know, I doubt this will be a common problem among the entire user base.

That all being said I see two possible routes. The first is that you register your own "LazyImage" content type inside your application to wrap this type of logic. In which case you'd simply make "data" a method call instead of an attribute to avoid needing to change code in operations.rb. Using ||= you could cache the image data for future use instead of reading every time.

Alternatively, we could consider merging this PR if the "lazy loading" was toggle-able via a keyword passed in when setting the image up in the context hash.

@eneagoe
Copy link
Author

eneagoe commented Jul 18, 2019

Regarding using the same image twice in the same template, we have this scenario in our app, and there seems to be no problem there - the image is correctly inserted every time.

Also, I might not be correct, but the code at

def set_local_rid(env, image)
seems to cache the image the first time it's being generated, so it is being re-used every time it's referenced afterwards.

We did consider having a separate LazyImage handler, but in the end the current set of changes seemed more easy to maintain. However, it is up to you - we could try the keyword argument for explicitly requesting lazy loading, or even implementing a new LazyImage handler.

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