Add tests for function/class declared in a file included repeatedly #7563

mvorisek · 2021-10-06T10:32:34Z

Memory leaks are fixed in PHP 8.1 /w opcache.

Add tests to check if memory does not increase on repeated include so code can rely on it.

cmb69 · 2021-10-06T11:14:56Z

Memory leaks are fixed in PHP 8.1 /w opcache.

So the tests need to be skipped if OPcache is not enabled.

mvorisek · 2021-10-06T11:21:30Z

Memory leaks are fixed in PHP 8.1 /w opcache.

So the tests need to be skipped if OPcache is not enabled.

yes

cmb69 · 2021-10-06T11:34:28Z

Even if the extension is loaded, it may not be enabled.

Zend/tests/repeated_include/anonymous_class_in_closure.phpt

cmb69

Thanks for the PR! Frankly, I'm not sure whether it makes sense to have these tests, but they don't seem to hurt either.

Zend/tests/repeated_include/anonymous_class.phpt

cmb69

This is okay for me.

nikic

Including files more than twice is unnecessary.

mvorisek · 2021-10-08T15:14:57Z

Including files more than twice is unnecessary.

This is not true, for ex. in PHP 7.4 /w opcache I needed 82 iterations for anonymous_class.phpt (on other platform, I needed 143 iterations), full test output:

--TEST--
Repeated include must not increase memory - anonymous class
--SKIPIF--
--FILE--
Increased memory detected: 65536 B (82 iteration)

I belive this is because some hashmap resize was needed and these tests accounts for that. On linux, the tests are fast, on Windows, include is slower but the tests accounts for that to keep them fast.

mvorisek · 2021-11-24T14:05:53Z

@nikic is the iteration count in the test with this reasoning enoughly justified?

mvorisek · 2022-02-01T11:48:09Z

@cmb69 can these tests be merged?

cmb69 · 2022-02-01T13:51:07Z

I think @nikic is referring to our builds using MSan (and similar) which should be able to detect the memory leaks. As such, neither including more than twice should be necessary, nor checking the memory usage manually.

mvorisek · 2022-02-01T13:58:04Z

I do not think so, this is not related to memory leak of an object instance, but instead of a anonymous class DEFINITION. Correct me if I am wrong, but I do not think this is checked in MSan.

Also, as the used memory increase can be observed with much higher include count only, I belive the only way to test it reliably is to test is with reasonable count which caused issues in the prePHP8.1 versions.

mvorisek · 2022-02-19T13:22:05Z

The importance of more than several iterations can be seen in #8078 to detect memory leaks such op_array arena allocation.

Zend/tests/repeated_include/anonymous_class.phpt

Girgias · 2022-04-27T18:08:14Z

Why is this targeting 8.1 and not master?

mvorisek · 2022-04-27T18:12:49Z

Why is this targeting 8.1 and not master?

because the memory leakage was fixed in PHP 8.1 /w opcache, thus I target the lowest possible branch to assert that behaviour since this version

Zend/tests/repeated_include/anonymous_class.phpt

mvorisek · 2022-07-23T07:56:03Z

Can this PR be merged? It asserts no memory leaking which is quite important feature user may rely on.

mvorisek · 2022-11-30T12:01:16Z

I would like to summarize this PR:

/w opcache repeatedly included file does not leak (since PHP 8.1, hence the target branch)
/wo opcache there are some leaks (as @Girgias advised above, dynamic xfail is emit)
originally the memory leak was detectable with tens or several hundreds iterations, so the tests test many iterations, but they are fast, they complete below 1 second (@nikic originally said 2 iterations are enough, but many iterations are needed)

Given these facts, I would like to have this PR merged as I belive all feedback was addressed.

mvorisek · 2023-04-18T08:06:24Z

@nielsdos @iluuu1994 are you ok to merge these tests? And can they target PHP-8.1?

nielsdos · 2023-04-18T15:55:03Z

I don't know if all the variants are truely necessary, since it is unrelated to dynamic definitions. Other than that I guess testing that opcache doesn't leak this probably makes sense.
Since there is no fix here I believe the policy is to put it in master.
Idk what Ilija thinks.

iluuu1994

There's too little context to this PR. It's unclear what fix this is trying to test, and whether that's already covered by the tests added at the time.

iluuu1994 · 2023-04-20T06:59:57Z

Zend/tests/repeated_include/anonymous_class.phpt

+$m = -1;
+$mDiff = -1;
+$mPrev = 0;
+for ($i = 0; $i < (PHP_OS_FAMILY === 'Windows' ? 1_000 /* include is slow on Windows */ : 20_000); $i++) {


If 1,000 iterations are enough for Windows, why use 20,000 on others?

On some systems I needed several hundreds iterations to trigger a memory increase. So I figured 20k is good count to test this extensively and keep it still fast (<1 second), in Windows, the repeated include is quite costly, so on Windows, only 1k iterations are tested in hope it will be enough and to keep the test/CI still fast.

Zend/tests/repeated_include/anonymous_class.phpt

iluuu1994 · 2023-04-28T16:15:17Z

@mvorisek

There's too little context to this PR. It's unclear what fix this is trying to test, and whether that's already covered by the tests added at the time.

This hasn't been answered yet. Do you have a link to the original commit that fixes what we're testing here?

mvorisek · 2023-04-28T16:37:32Z

The explamation is in the tests titles. I do not think this is tested now, thus this PR to assert no memory increase on repeated include with opcache. Once fixed for /wo opcache, SKIPIF should be removed.

mvorisek · 2023-05-18T14:02:25Z

I hope we can agree memory leaks are not wanted. The current impl. with opcache does not leak, so we test/assert it. For non-opcache we xfail the tests now. I hope this makes sense.

mvorisek · 2023-08-16T10:27:02Z

@iluuu1994 can this PR be merged please? It asserts the expected behaviour - currently with opcache only - discussed in #11975

derickr · 2023-09-28T11:53:55Z

I'm closing this as there is no reason this should target PHP-8.1. The requested information and changes have also not been addressed.

mvorisek · 2023-10-11T09:50:55Z

Folks, could you please tell me what I could do so that this PR gets merged? Let me know if you have any concerns

mvorisek force-pushed the include_repeat_leak branch 2 times, most recently from 08aed71 to 1fc627a Compare October 6, 2021 12:00

cmb69 reviewed Oct 6, 2021

View reviewed changes

Zend/tests/repeated_include/anonymous_class_in_closure.phpt Outdated Show resolved Hide resolved

mvorisek force-pushed the include_repeat_leak branch 2 times, most recently from 84e0695 to 2f0d35c Compare October 6, 2021 14:20

mvorisek requested a review from cmb69 October 6, 2021 16:49

cmb69 reviewed Oct 6, 2021

View reviewed changes

Zend/tests/repeated_include/anonymous_class.phpt Outdated Show resolved Hide resolved

mvorisek force-pushed the include_repeat_leak branch from 651c753 to e1795fe Compare October 6, 2021 18:01

mvorisek requested a review from cmb69 October 7, 2021 01:14

krakjoe added Category: Tests Waiting on Review labels Oct 7, 2021

cmb69 approved these changes Oct 8, 2021

View reviewed changes

nikic requested changes Oct 8, 2021

View reviewed changes

mvorisek requested a review from nikic October 8, 2021 18:12

mvorisek mentioned this pull request Feb 11, 2022

Fix memory leak of function attribute hash table #8070

Closed

mvorisek force-pushed the include_repeat_leak branch from e1795fe to 153c4ab Compare April 12, 2022 10:31

mvorisek commented Apr 12, 2022

View reviewed changes

Zend/tests/repeated_include/anonymous_class.phpt Outdated Show resolved Hide resolved

mvorisek force-pushed the include_repeat_leak branch 2 times, most recently from d66436c to 2cf0a67 Compare April 12, 2022 12:30

mvorisek mentioned this pull request Apr 12, 2022

Fix opcache testing in CI #8350

Closed

mvorisek force-pushed the include_repeat_leak branch from 2cf0a67 to 9959470 Compare April 12, 2022 13:06

mvorisek force-pushed the include_repeat_leak branch from 9959470 to 17970f0 Compare April 14, 2022 08:20

mvorisek requested a review from iluuu1994 April 14, 2022 08:23

Girgias requested changes May 18, 2022

View reviewed changes

Zend/tests/repeated_include/anonymous_class.phpt Outdated Show resolved Hide resolved

mvorisek mentioned this pull request Oct 23, 2022

memory usage increasing when using opcache.enable_cli and opcache.file_cache_only #9812

Open

mvorisek mentioned this pull request Nov 19, 2022

Line Coverage: mark each line of the Branch as executable sebastianbergmann/php-code-coverage#959

Closed

mvorisek force-pushed the include_repeat_leak branch from 17970f0 to 1bbf4fa Compare November 19, 2022 15:15

mvorisek requested review from Girgias and removed request for nikic and iluuu1994 November 19, 2022 16:05

iluuu1994 reviewed Apr 20, 2023

View reviewed changes

Add tests for function/class declared in a file included repeatedly

7f54b3d

mvorisek force-pushed the include_repeat_leak branch from 1bbf4fa to 7f54b3d Compare April 20, 2023 15:10

mvorisek mentioned this pull request Aug 15, 2023

Anonymous class names change based on the opcache status of the PHP file #11975

Closed

derickr closed this Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for function/class declared in a file included repeatedly #7563

Add tests for function/class declared in a file included repeatedly #7563

mvorisek commented Oct 6, 2021 •

edited

Loading

cmb69 commented Oct 6, 2021

mvorisek commented Oct 6, 2021 •

edited

Loading

cmb69 commented Oct 6, 2021

cmb69 left a comment

cmb69 left a comment

nikic left a comment

mvorisek commented Oct 8, 2021 •

edited

Loading

mvorisek commented Nov 24, 2021

mvorisek commented Feb 1, 2022

cmb69 commented Feb 1, 2022

mvorisek commented Feb 1, 2022

mvorisek commented Feb 19, 2022

Girgias commented Apr 27, 2022

mvorisek commented Apr 27, 2022

mvorisek commented Jul 23, 2022

mvorisek commented Nov 30, 2022

mvorisek commented Apr 18, 2023

nielsdos commented Apr 18, 2023

iluuu1994 left a comment

iluuu1994 Apr 20, 2023

mvorisek Apr 20, 2023 •

edited

Loading

iluuu1994 commented Apr 28, 2023

mvorisek commented Apr 28, 2023

mvorisek commented May 18, 2023

mvorisek commented Aug 16, 2023

derickr commented Sep 28, 2023

mvorisek commented Oct 11, 2023

Add tests for function/class declared in a file included repeatedly #7563

Add tests for function/class declared in a file included repeatedly #7563

Conversation

mvorisek commented Oct 6, 2021 • edited Loading

cmb69 commented Oct 6, 2021

mvorisek commented Oct 6, 2021 • edited Loading

cmb69 commented Oct 6, 2021

cmb69 left a comment

Choose a reason for hiding this comment

cmb69 left a comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

mvorisek commented Oct 8, 2021 • edited Loading

mvorisek commented Nov 24, 2021

mvorisek commented Feb 1, 2022

cmb69 commented Feb 1, 2022

mvorisek commented Feb 1, 2022

mvorisek commented Feb 19, 2022

Girgias commented Apr 27, 2022

mvorisek commented Apr 27, 2022

mvorisek commented Jul 23, 2022

mvorisek commented Nov 30, 2022

mvorisek commented Apr 18, 2023

nielsdos commented Apr 18, 2023

iluuu1994 left a comment

Choose a reason for hiding this comment

iluuu1994 Apr 20, 2023

Choose a reason for hiding this comment

mvorisek Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

iluuu1994 commented Apr 28, 2023

mvorisek commented Apr 28, 2023

mvorisek commented May 18, 2023

mvorisek commented Aug 16, 2023

derickr commented Sep 28, 2023

mvorisek commented Oct 11, 2023

mvorisek commented Oct 6, 2021 •

edited

Loading

mvorisek commented Oct 6, 2021 •

edited

Loading

mvorisek commented Oct 8, 2021 •

edited

Loading

mvorisek Apr 20, 2023 •

edited

Loading