Skip to content

Specify unit in out of memory error #8820

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

Merged
merged 1 commit into from
Jun 21, 2022
Merged

Specify unit in out of memory error #8820

merged 1 commit into from
Jun 21, 2022

Conversation

iluuu1994
Copy link
Member

Closes GH-8808

@cmb69
Copy link
Member

cmb69 commented Jun 17, 2022

I'm not too happy with the error message generally, and duplicating "bytes" doesn't make it better, in my opinion. Can't we come up with something else? OTOH, certainly not a hill to die upon for me.

@iluuu1994
Copy link
Member Author

I'll try to come up with something better.

@mvorisek
Copy link
Contributor

also the number should be converted to human format (with KB, MB, GB, ... unit)

#else
zend_mm_safe_error(heap, "Out of memory (allocated %zu) (tried to allocate %zu bytes)", heap->real_size, ZEND_MM_PAGE_SIZE * pages_count);
zend_mm_safe_error(heap, "Out of memory (allocated %zu bytes) (tried to allocate %zu bytes)", heap->real_size, ZEND_MM_PAGE_SIZE * pages_count);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
zend_mm_safe_error(heap, "Out of memory (allocated %zu bytes) (tried to allocate %zu bytes)", heap->real_size, ZEND_MM_PAGE_SIZE * pages_count);
zend_mm_safe_error(heap, "Out of memory while trying to allocate %zu bytes (allocated %zu bytes)", ZEND_MM_PAGE_SIZE * pages_count, heap->real_size);

Maybe?

@Girgias Girgias merged commit cd363a9 into php:master Jun 21, 2022
@Girgias
Copy link
Member

Girgias commented Jun 21, 2022

I've merged because we can always improve the error message later

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.

better error reporting
5 participants