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

setResourceLimit, PHP7 and 32bit #200

Open
Y0lk opened this issue Feb 7, 2017 · 8 comments
Open

setResourceLimit, PHP7 and 32bit #200

Y0lk opened this issue Feb 7, 2017 · 8 comments

Comments

@Y0lk
Copy link
Contributor

Y0lk commented Feb 7, 2017

Since PHP7's type declarations, setResourceLimit's 2nd argument is declared as int. That's not so much a problem on 64bit systems but on 32bit, you're now limited 2147483647. If you're setting Imagick::RESOURCETYPE_DISK for example, since this has to be set in bytes, you're essentially limited to ~2.1gb, and that's unfortunately too low if you're manipulating big images.

Before, PHP would interpret it as float and it would work just fine with Imagick. Now, you get a warning about expecting an int so there's no way to set the resource limits higher if you're using PHP in 32bit.

@Danack
Copy link
Collaborator

Danack commented Feb 7, 2017

Yeah, that would be a problem.

I should be able to change the function to accept a float, rather than int, and that will work up to about 2^53.....but that is rather a nasty hack. I'll have a think to see if there's a more sensible way of doing, before committing that.

@Y0lk
Copy link
Contributor Author

Y0lk commented Feb 8, 2017

Actually, I said it happens since PHP7 but I just tested it on php5.6 with the same imagick version (on 3.4.3RC1 atm) and it gets typecasted to a 32bit int so you still end up with the same problem, (except that it overflows instead of giving you a warning). I guess it changed in a certain imagick update then? I didn't do any new tests with an old imagick version but it definitely worked before.

@Llbe
Copy link

Llbe commented Feb 10, 2017

Just a thought: what about a new parameter for setting the unit?

Defaulting to bytes for BC.

@Danack
Copy link
Collaborator

Danack commented Feb 10, 2017

@libe Yeah I was thinking of something similar....however that is equally a hack, just in a different way.

For example if we used just kilobytes as the unit, then it would be possible to set 4 GB as the limit with (1,048,576 * KB) but it wouldn't be possible to set 1,073,741,312 (4GB - 512) as it's not a multiple of the unit.

The hack with the float would be accurate up to 2^53 which is 9,007,199,254,740,992 or 8,388,608 GB. Or if the call to set the resource limit was specifying an image width, that would be higher resolution than needed to cover the whole planet at 1 million DPI.

Both of those sound 'adequate' range.

@Y0lk
Copy link
Contributor Author

Y0lk commented Feb 10, 2017

I think I like the idea of floats more. Having the unit might be confusing since setResourceLimit is used for other things like threads. About that, I just noticed the doc says "Sets the limit for a particular resource in megabytes" which is not the case and doesn't really make sense if you're setting something else than memory (like threads). I'll update the doc when whatever change is made :)

@Danack
Copy link
Collaborator

Danack commented Feb 14, 2017

Hi @Y0lk,

This should be fixed since this commit.

Do you have the capability to test it? I don't have a 32bit environment to test against currently.

@Y0lk
Copy link
Contributor Author

Y0lk commented Feb 15, 2017

Hi @Danack,
Just tested this. Works but getResourceLimit still returned an int (so value was returned incorrectly on 32bit). I made a quick pull request : #202

@Y0lk
Copy link
Contributor Author

Y0lk commented Feb 15, 2017

Actually, it broke the setresourcelimit test because the values are not the same type. Typecasted the provided value to a float since this is what setResourceLimit expects anyway and is what getResourceLimit should return.

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

No branches or pull requests

3 participants