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

provide an option to set the default stack size on linux #1291

Merged
merged 1 commit into from Apr 20, 2019

Conversation

abdollar
Copy link
Contributor

@abdollar abdollar commented Apr 19, 2019

  • Added a new environment variable called VIPS_MIN_STACK_SIZE, so that we can set the default stack size on some linux platforms e.g alpine, which uses musl and assumes a lower default stack size for embedded systems and containers. Some libraries are heavy users of a larger stack size such as poppler for PDF conversion.
  • This is only set on linux and only if the environment variable "VIPS_MIN_STACK_SIZE" is set and is higher than the current stack size.
  • We default to 2MB if we cannot parse this value.
  • Added an error message with the current stack size.
  • Tested on a PDF to png conversion that used around 90k stack for a recursive call, which is in connection to issue Segmentation fault for specific pdf on alpine linux docker container #1287

e.g it can be used like so, if you pass a string it defaults to 2MB otherwise the value passed.

VIPS_MIN_STACK_SIZE=default vips copy stackheavy.pdf x.png
VIPS_MIN_STACK_SIZE=2m vips copy stackheavy.pdf x.png

If you try to set it too low you see the following warning

process:60882): VIPS-WARNING **: 05:32:00.857: Could not set minimum pthread stack size of 2k, current size is 80k

@jcupitt
Copy link
Member

jcupitt commented Apr 19, 2019

Hello @abdollar,

This feels ugly! This kind of thing should not be set by libraries. But I agree that (like that horrible stdio hack for Windows) it makes pragmatic sense.

How about checking the current stacksize and making sure we don't lower it? There would then be less danger of fighting with some other library over stacksize. The env var could perhaps be renamed VIPS_MIN_STACK_SIZE.

I would use guint64 vips__parse_size( const char *size_string ) instead of atoi(). It'll allow strings like "4m".

I agree, 2m seems a sensible minimum.

@abdollar
Copy link
Contributor Author

@jcupitt - updated changes and description per your suggestions of VIPS_MIN_STACK_SIZE. I added a warning if the current stack size is larger indicating we are not going to change it and included the current settings. So - I was debating adding this and going back and forth on this change. It seems to make sense due to the init section and thread pool being managed here and it feels right especially for those of us with containers and environment variables. Thanks.

@lovell
Copy link
Member

lovell commented Apr 19, 2019

Does the stack size need to be aligned to the page size?

https://linux.die.net/man/3/pthread_attr_setstacksize
https://linux.die.net/man/2/getpagesize

@jcupitt
Copy link
Member

jcupitt commented Apr 19, 2019

I found this issue discussing the 80kb limit and how to step around it. There are interesting links to the solutions other projects have cooked up.

https://github.com/voidlinux/void-packages/issues/4147

Our case is slightly different from the large stack frames allocations they discuss. Poppler walks the document structure recursively, so the low limit simply makes some common user input impossible to handle.

@lovell I think you need to be page-aligned for setstack, but the stack size does not need to be.

It looks like not all pthread implementations have pthread_setattr_default_np() -- musl added this in 2016, for example. We probably need to add a test to configure. @abdollar are you OK adding a configure test? I can do this if you'd rather not.

@abdollar
Copy link
Contributor Author

@jcupitt - I have updated configure and added a library check to include HAVE_PTHREAD_DEFAULT_NP using AC_CHECK_LIB([pthread]. I removed the linux check, but could add it back, though this seems to make more sense for any thing with pthreads and pthread_setattr_default_np.

@jcupitt
Copy link
Member

jcupitt commented Apr 19, 2019

Nice! Looks good to me, anyone have any other comments?

@jcupitt jcupitt merged commit 36bd9df into libvips:master Apr 20, 2019
@jcupitt
Copy link
Member

jcupitt commented Apr 20, 2019

OK, let's merge! Thank you for working on this, @abdollar, it's a nice improvement.

jcupitt added a commit that referenced this pull request Apr 20, 2019
- need to define _GNU_SOURCE for glibc to get pthread_setattr_default_np()
- don't warn if we already have enough stack
- reformat to libvips standard
- add note to docs

see: #1291
@jcupitt
Copy link
Member

jcupitt commented Apr 20, 2019

I added _GNU_SOURCE to get it to work on glibc. I added a note to the docs, stopped the message if the stack is already large enough, and did a minor reformat to libvips conventions.

priyankatapar pushed a commit to priyankatapar/libvips that referenced this pull request Apr 22, 2019
provide an option to set the default stack size on linux
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

3 participants