Skip to content

Fixes memory problem of getBodySize when provided with large BodySize#184

Merged
staabm merged 1 commit into
stomp-php:masterfrom
ngivanyh:master
May 21, 2025
Merged

Fixes memory problem of getBodySize when provided with large BodySize#184
staabm merged 1 commit into
stomp-php:masterfrom
ngivanyh:master

Conversation

@ngivanyh
Copy link
Copy Markdown
Contributor

An alternate way of calculating the BodySize, using less memory.

@staabm
Copy link
Copy Markdown
Member

staabm commented May 20, 2025

which problem are you trying to solve? can you add a unit test?

@ngivanyh
Copy link
Copy Markdown
Contributor Author

To reiterate:

Implementations to Compare (Replace FILE with desired file to test with)

before.php (The current implementation of getBodySize()):

<?php
$file = file_get_contents('FILE');

echo memory_get_usage() . "\n";
echo count(unpack('C*', $file)) . "\n";
echo memory_get_usage() . "\n";

after.php (Proposal):

<?php
$file = file_get_contents('FILE');

echo memory_get_usage() . "\n";
echo strlen($file) . "\n";
echo memory_get_usage() . "\n";

Setup

PHP Memory Limit:

memory_limit => 128M => 128 (php -i | grep memory_limit) (php 8.4.7)

Creation of Files:

truncate -s SIZE fileSIZE (Replace size with desired size)

End result (ls -alh):

-rw-r--r--@  1 user  staff    10M May 20 21:33 file1
-rw-r--r--@  1 user  staff    10M May 20 21:33 file5
-rw-r--r--@  1 user  staff    10M May 20 21:33 file10
-rw-r--r--@  1 user  staff    20M May 20 21:33 file20
-rw-r--r--@  1 user  staff    30M May 20 21:33 file30
-rw-r--r--@  1 user  staff    40M May 20 21:33 file40
-rw-r--r--@  1 user  staff    50M May 20 21:32 file50

The Results

File Size (MB) Before Change (before.php) After Change (after.php)
10 PHP Fatal error: Allowed memory size of 134217728 bytes exhausted
20 PHP Fatal error: Allowed memory size of 134217728 bytes exhausted
30 PHP Fatal error: Allowed memory size of 134217728 bytes exhausted
40 PHP Fatal error: Allowed memory size of 134217728 bytes exhausted
50 PHP Fatal error: Allowed memory size of 134217728 bytes exhausted

Proof that strlen($file) Works:

File Size (MB) Before Change (before.php) After Change (after.php)
1
5 PHP Fatal error: Allowed memory size of 134217728 bytes exhausted
File Size (MB) Output of before.php Output of after.php
1 1448272\n1048576\n1448304 1447976\n1048576\n1448008
5 5654592\n5242880\n5654624

Note: The \ns are newlines, they were just changed to fit into the table

Verdict

Feel free to experiment and look at the results for yourself, but on my end, they speak for themselves. Both methods return the same size. But one can handle far larger sizes than the other, without failing. At smaller file sizes, the benefits are minuscule, however, by testing with larger and larger files, the difference is soon evident.

@staabm
Copy link
Copy Markdown
Member

staabm commented May 20, 2025

running the before/after example you suggsted on my machine leads identical results

➜  php-rocket git:(master) ✗ cat test.php         
<?php
$file = file_get_contents('FILE');

echo memory_get_usage() . "\n";
echo count(unpack('C*', $file)) . "\n";
echo memory_get_usage() . "\n";
➜  php-rocket git:(master) ✗ php test.php
31868352
31457280
31868384
➜  php-rocket git:(master) ✗ cat test.php
<?php
$file = file_get_contents('FILE');

echo memory_get_usage() . "\n";
echo strlen($file) . "\n";
echo memory_get_usage() . "\n";
➜  php-rocket git:(master) ✗ php test.php
31868056
31457280
31868088
➜  php-rocket git:(master) ✗ ls -la FILE                                 
-rw-r--r--@ 1 m.staab  staff  31457280 May 20 17:00 FILE
➜  php-rocket git:(master) ✗ 

➜  php-rocket git:(master) ✗ php -v
PHP 8.3.21 (cli) (built: May  6 2025 13:58:10) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.3.21, Copyright (c) Zend Technologies

@ngivanyh
Copy link
Copy Markdown
Contributor Author

Have you tried different files (with increasing sizes) other than FILE? Because if so, there should be the memory limit reached error between sizes of 10 - 50 MB (I've tested on different computers with this same test, but on the other one i've tested, before.php only started failing at roughly 40 something MB, although I didn't check the installed version of php and the memory limit on that one).

@staabm
Copy link
Copy Markdown
Member

staabm commented May 21, 2025

ok, I did a few more tests using time php -d memory_limit=128M test.php

misleading was the output of memory_get_usage as this is not related to the improvement of the patch itself.

with a 80MB file on macOS I can reproduce a fatal error and also that the new code is considerably faster:

➜  php-rocket git:(master) ✗ time php old.php
84297152
83886080
84297184
php test.php  1.22s user 0.33s system 90% cpu 1.713 total

➜  php-rocket git:(master) ✗ time php new.php
84296856
83886080
84296888
php test.php  0.01s user 0.02s system 89% cpu 0.034 total

➜  php-rocket git:(master) ✗ time php -d memory_limit=128M test.php 
84297152
PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 33554440 bytes) in /Users/m.staab/Documents/GitHub/php-rocket/test.php on line 5

Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 33554440 bytes) in /Users/m.staab/Documents/GitHub/php-rocket/test.php on line 5
php -d memory_limit=128M test.php  0.03s user 0.02s system 84% cpu 0.060 total

@staabm
Copy link
Copy Markdown
Member

staabm commented May 21, 2025

did you test your change with different real world files of different formats?

@ngivanyh
Copy link
Copy Markdown
Contributor Author

ngivanyh commented May 21, 2025

Environment: Same as above.

Images (.jpg):

ID Size before.php after.php Resolution
1 8.7M 7680x4320 (8K)
2 7.4M 3840x2160 (4K)
3 490K 1920x1080 (2K)

Text (.txt):

Creation: This

ID Size (MB) before.php after.php
1 1
2 5
3 10

Note: There is also a speedup in completion time across the board. (time php before/after.php)

Two common file formats with real world usage tested. Returns the same number, one just can handle higher file sizes without reaching the memory limit of 128M, alongside a speed benefit.

Copy link
Copy Markdown
Member

@staabm staabm left a comment

Choose a reason for hiding this comment

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

Thanks

@staabm staabm merged commit 2c9573a into stomp-php:master May 21, 2025
4 of 5 checks passed
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.

2 participants