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

NEW Remove ImageBackendFactory Injector override from ImageThumbnailHelper #905

Conversation

maxime-rainville
Copy link
Contributor

ImageThumbnailHelper overrides ImageBackendFactory with InjectionCreator. This seems to increase the memory usage systematically when resizing big pictures. Not quite sure why, but I think some object hang around when InjectionCreator.

This doesn't actually show when calling memory_get_peak_usage because the memory is actually used by some process outside of PHP that's responsible for resizing the image.

Parent issue

@maxime-rainville
Copy link
Contributor Author

@flamerohr You wrote the original ImageThumbnailHelper class. It's not clear to me why we are registering a different service for ImageBackendFactory. Do you remember why?

@maxime-rainville maxime-rainville changed the title WIP Remove ImageBackendFactory Injector override from ImageThumbnailHelper NEW Remove ImageBackendFactory Injector override from ImageThumbnailHelper Jan 23, 2019
code/Helper/ImageThumbnailHelper.php Outdated Show resolved Hide resolved
Copy link
Contributor

@bergice bergice left a comment

Choose a reason for hiding this comment

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

I'm gonna have a go at profiling this before I approve it.

maxime-rainville pushed a commit to open-sausages/silverstripe-asset-admin that referenced this pull request Jan 29, 2019
@maxime-rainville maxime-rainville force-pushed the pulls/1/optimise-image-backend-to-reduce-memory-usage branch from 4535dbe to ca5e80b Compare January 29, 2019 02:16
@maxime-rainville maxime-rainville changed the base branch from pulls/1/optimise-image-backend-to-reduce-memory-usage to 1 January 29, 2019 02:19
@bergice bergice merged commit f29848b into silverstripe:1 Jan 29, 2019
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

Some minor feedback

/**
* @var int
*/
private $maxImageFileSize;
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer protected over private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the context that there's a proper accessor and mutator, I think it's better to make it private. My view is people should be using those instead of accessing the variable directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

We really need to clarify this policy - consistency is the most important thing in my mind, but we've always gone with protected over private until the last couple of months

private $maxImageFileSize;

/**
* @param mixed $maxImageSize Maximum file size for which thumbnails will be generated. Set to `0` to disable the
Copy link
Contributor

Choose a reason for hiding this comment

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

Mixed is not a useful annotation - I think string|int would be better

code/Helper/ImageThumbnailHelper.php Show resolved Hide resolved

/**
* Set the maximum file size for which thumbnails will be generated. Set to `0` to disable the limit.
* @param mixed $size
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest string|int

@maxime-rainville maxime-rainville deleted the pulls/1/optimise-image-backend-to-reduce-memory-usage branch January 29, 2019 22:28
maxime-rainville pushed a commit to open-sausages/silverstripe-asset-admin that referenced this pull request Jan 29, 2019
@maxime-rainville
Copy link
Contributor Author

@robbieaverill I've addressed your feedback in #910

kinglozzer added a commit that referenced this pull request Jan 30, 2019
…kend-to-reduce-memory-usage

MINOR Implement some of the feedback that got missed from #905
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants