[Config] Early return for DirectoryResource #21458

robfrawley · 2017-01-29T22:00:02Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	sure?
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets
Related PRs	#21440
License	MIT
Doc PR	n/a

Alternate PR that implements an early return for DirectoryResource to increase the speed on large file sets. We can never return early with true without checking all assets within the resource, as the aforementioned referenced PR did; hence this PR takes the counter approach and returns false early where appropriate.

Conversation about possible bug at #21458 (comment).

ogizanagi · 2017-01-29T22:14:21Z

src/Symfony/Component/Config/Resource/DirectoryResource.php

+                return false;
+            }
+
+            $newestMTime = max($fileMTime, $newestMTime);


This can be removed, right?

Also, may test directory mtime first (before the foreach), and return false if greater than passed timestamp.

No, removing that breaks the test ->isFresh() returns false if an existing file is removed. Yes, though, we could add another condition at the very beginning.

Added initial conditional before loop.

Let me know if I'm wrong, but I think the test is failing in this case because the implementation and the test case are slightly flawed.

Try:

diff --git a/src/Symfony/Component/Config/Resource/DirectoryResource.php b/src/Symfony/Component/Config/Resource/DirectoryResource.php index dcb5e19..bef0b2b 100644 --- a/src/Symfony/Component/Config/Resource/DirectoryResource.php +++ b/src/Symfony/Component/Config/Resource/DirectoryResource.php @@ -95,7 +95,7 @@ class DirectoryResource implements SelfCheckingResourceInterface, \Serializable $newestMTime = max($fileMTime, $newestMTime); } - return $newestMTime < $timestamp; + return $newestMTime <= $timestamp; } public function serialize()

and the test case should fail. However, I'd expect a resource to be considered fresh if its mtime is the exact same as the provided timestamp (on the contrary, you should revised the new conditions to use $timestamp <= $fileMTime when returning false).

The following for instance should work:

See patch

diff --git a/src/Symfony/Component/Config/Resource/DirectoryResource.php b/src/Symfony/Component/Config/Resource/DirectoryResource.php index dcb5e19..deb40b0 100644 --- a/src/Symfony/Component/Config/Resource/DirectoryResource.php +++ b/src/Symfony/Component/Config/Resource/DirectoryResource.php @@ -74,7 +74,10 @@ class DirectoryResource implements SelfCheckingResourceInterface, \Serializable return false; } - $newestMTime = filemtime($this->resource); + if($timestamp < $newestMTime = filemtime($this->resource)) { + return false; + }; + foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($this->resource), \RecursiveIteratorIterator::SELF_FIRST) as $file) { // if regex filtering is enabled only check matching files if ($this->pattern && $file->isFile() && !preg_match($this->pattern, $file->getBasename())) { @@ -88,14 +91,12 @@ class DirectoryResource implements SelfCheckingResourceInterface, \Serializable } // early return if any file mtime is greater than passed timestamp - if (($fileMTime = $file->getMTime()) > $timestamp) { + if ($timestamp < $fileMTime = $file->getMTime()) { return false; } - - $newestMTime = max($fileMTime, $newestMTime); } - return $newestMTime < $timestamp; + return true; } public function serialize() diff --git a/src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php b/src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php index a22209d..9d8e055 100644 --- a/src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php +++ b/src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php @@ -110,8 +110,10 @@ class DirectoryResourceTest extends \PHPUnit_Framework_TestCase public function testIsFreshDeleteFile() { $resource = new DirectoryResource($this->directory); + $time = time(); + sleep(1); unlink($this->directory.'/tmp.xml'); - $this->assertFalse($resource->isFresh(time()), '->isFresh() returns false if an existing file is removed'); + $this->assertFalse($resource->isFresh($time), '->isFresh() returns false if an existing file is removed'); } public function testIsFreshDeleteDirectory()

Do we consider changing this a BC break, though? That's why I hadn't implemented something similar in my testing leading up to this PR. Assuming the answer is no, I'll happily implement these changes, though @nicolas-grekas @ogizanagi

It's rather a bug fix to me.
For reference, FileResource uses filemtime($this->resource) <= $timestamp too.

Something like this greatly simplifies the code in this PR, and I think may be a viable option (this diff is against this PR), without changing the tests at all:

diff --git a/src/Symfony/Component/Config/Resource/DirectoryResource.php b/src/Symfony/Component/Config/Resource/DirectoryResource.php index 9011cfe..501f8a5 100644 --- a/src/Symfony/Component/Config/Resource/DirectoryResource.php +++ b/src/Symfony/Component/Config/Resource/DirectoryResource.php @@ -68,7 +68,7 @@ class DirectoryResource implements ResourceInterface, \Serializable return false; } - if ($timestamp < $newestMTime = filemtime($this->resource)) { + if ($timestamp <= filemtime($this->resource)) { return false; } @@ -85,14 +85,12 @@ class DirectoryResource implements ResourceInterface, \Serializable } // early return if a file's mtime exceeds the passed timestamp - if ($timestamp < $fileMTime = $file->getMTime()) { + if ($timestamp <= $file->getMTime()) { return false; } - - $newestMTime = max($fileMTime, $newestMTime); } - return $newestMTime < $timestamp; + return true; } public function serialize()

Let's see what the maintainers say; changes pushed.

I too think that the same timestamp should be considered fresh. Can you clarify why the behavior exists such that a matching timestamp is not fresh @nicolas-grekas

should be fresh with same timestamp to me also - same as in FileResource

nicolas-grekas · 2017-01-29T22:44:46Z

src/Symfony/Component/Config/Resource/DirectoryResource.php

@@ -68,7 +68,10 @@ public function isFresh($timestamp)
            return false;
        }

-        $newestMTime = filemtime($this->resource);
+        if (($newestMTime = filemtime($this->resource)) > $timestamp) {


You can swap for a <, and remove the brackets as done usually

Same below btw

nicolas-grekas

👍 with minor typo

nicolas-grekas · 2017-01-29T22:55:12Z

src/Symfony/Component/Config/Resource/DirectoryResource.php

@@ -81,7 +84,12 @@ public function isFresh($timestamp)
                continue;
            }

-            $newestMTime = max($file->getMTime(), $newestMTime);
+            // early return if a file's mtime is exceeds than passed timestamp


is exceeds =>

xabbuh

👍

nicolas-grekas · 2017-01-30T13:04:35Z

src/Symfony/Component/Config/Resource/DirectoryResource.php

@@ -68,7 +68,10 @@ public function isFresh($timestamp)
            return false;
        }

-        $newestMTime = filemtime($this->resource);
+        if ($timestamp <= filemtime($this->resource)) {


this needs to be turned to a <: the "equals" case should not return false, as in eg FileResource

It returned false before. Do we consider this behaviour change a bug fix?

Check second commit (can squash if its good)

nicolas-grekas · 2017-01-30T13:04:41Z

src/Symfony/Component/Config/Resource/DirectoryResource.php

@@ -81,10 +84,13 @@ public function isFresh($timestamp)
                continue;
            }

-            $newestMTime = max($file->getMTime(), $newestMTime);
+            // early return if a file's mtime exceeds the passed timestamp
+            if ($timestamp <= $file->getMTime()) {


Check second commit (can squash if its good)

robfrawley · 2017-01-30T18:36:08Z

src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php

        unlink($this->directory.'/tmp.xml');
-        $this->assertFalse($resource->isFresh(time()), '->isFresh() returns false if an existing file is removed');
+        $this->assertFalse($resource->isFresh($time), '->isFresh() returns false if an existing file is removed');


@nicolas-grekas This fails with the "bug fix" to not return false when timestamp are the same (unlike the original implementation). This test change fixes failures originating from this behavioral change...

Shouldn't we move this test into the time-sensitive group?

Good catch; I didn't know about that convention. I only see that group at the class-level. Is it okay to use it at the test-level?

Yes, that should work the same.

Adding that group causes the test to fail. What does that group do and why would it result in it failing?

@xabbuh We can't use that because the internal implementation only relates to the PHP interpreter. This relies on the filesystem and is external of the interpreter.

An added bonus of using the ClockMock class is that time passes instantly. Using PHP's sleep(10) will make your test wait for 10 actual seconds (more or less). In contrast, the ClockMock class advances the internal clock the given number of seconds without actually waiting that time, so your test will execute 10 seconds faster.
http://symfony.com/blog/new-in-symfony-2-8-clock-mocking-and-time-sensitive-tests

fabpot · 2017-02-12T19:13:07Z

Thank you @robfrawley.

This PR was squashed before being merged into the 2.7 branch (closes #21458). Discussion ---------- [Config] Early return for DirectoryResource | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | sure? | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | | Related PRs | #21440 | License | MIT | Doc PR | n/a Alternate PR that implements an early return for `DirectoryResource` to increase the speed on large file sets. We can never return early with `true` without checking all assets within the resource, as the aforementioned referenced PR did; hence this PR takes the counter approach and returns `false` early where appropriate. _Conversation about possible bug at #21458 (comment) Commits ------- d5746ec fix directory resource considers same timestamp not fresh 96107e2 return false early from directory resource

ogizanagi reviewed Jan 29, 2017

View reviewed changes

robfrawley force-pushed the feature-earily-directory-resource-return branch 2 times, most recently from cab5763 to fc1726f Compare January 29, 2017 22:25

robfrawley changed the base branch from master to 2.7 January 29, 2017 22:25

robfrawley force-pushed the feature-earily-directory-resource-return branch from fc1726f to d9701bd Compare January 29, 2017 22:31

robfrawley changed the title ~~Early return for DirectoryResource~~ [Config] Early return for DirectoryResource Jan 29, 2017

nicolas-grekas reviewed Jan 29, 2017

View reviewed changes

robfrawley force-pushed the feature-earily-directory-resource-return branch from d9701bd to 97bda48 Compare January 29, 2017 22:52

nicolas-grekas approved these changes Jan 29, 2017

View reviewed changes

robfrawley force-pushed the feature-earily-directory-resource-return branch 4 times, most recently from 8fabc4f to 5eac053 Compare January 30, 2017 00:04

return false early from directory resource

96107e2

robfrawley force-pushed the feature-earily-directory-resource-return branch from 5eac053 to 96107e2 Compare January 30, 2017 00:05

xabbuh approved these changes Jan 30, 2017

View reviewed changes

nicolas-grekas reviewed Jan 30, 2017

View reviewed changes

nicolas-grekas added this to the 2.7 milestone Jan 30, 2017

fix directory resource considers same timestamp not fresh

d5746ec

robfrawley commented Jan 30, 2017

View reviewed changes

nicolas-grekas added the Status: Reviewed label Feb 2, 2017

fabpot merged commit d5746ec into symfony:2.7 Feb 12, 2017

fabpot mentioned this pull request Feb 17, 2017

Release v3.2.4 #21640

Merged

This was referenced Mar 6, 2017

Release v2.7.25 #21884

Merged

Release v2.8.18 #21885

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Config] Early return for DirectoryResource #21458

[Config] Early return for DirectoryResource #21458

robfrawley commented Jan 29, 2017 •

edited

ogizanagi Jan 29, 2017 •

edited

robfrawley Jan 29, 2017

robfrawley Jan 29, 2017

ogizanagi Jan 29, 2017 •

edited

robfrawley Jan 29, 2017 •

edited

ogizanagi Jan 29, 2017

robfrawley Jan 29, 2017 •

edited

robfrawley Jan 29, 2017

robfrawley Jan 29, 2017 •

edited

nicolas-grekas Jan 30, 2017

nicolas-grekas Jan 29, 2017

nicolas-grekas Jan 29, 2017

robfrawley Jan 29, 2017

nicolas-grekas left a comment

nicolas-grekas Jan 29, 2017

robfrawley Jan 29, 2017

xabbuh left a comment

nicolas-grekas Jan 30, 2017 •

edited

xabbuh Jan 30, 2017

nicolas-grekas Jan 30, 2017

robfrawley Jan 30, 2017

nicolas-grekas Jan 30, 2017

robfrawley Jan 30, 2017

robfrawley Jan 30, 2017 •

edited

xabbuh Jan 30, 2017

robfrawley Jan 30, 2017

xabbuh Jan 30, 2017

robfrawley Jan 30, 2017

robfrawley Jan 30, 2017 •

edited

fabpot commented Feb 12, 2017

[Config] Early return for DirectoryResource #21458

[Config] Early return for DirectoryResource #21458

Conversation

robfrawley commented Jan 29, 2017 • edited

ogizanagi Jan 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi Jan 29, 2017 • edited

Choose a reason for hiding this comment

robfrawley Jan 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Jan 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Jan 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh left a comment

Choose a reason for hiding this comment

nicolas-grekas Jan 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Jan 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robfrawley Jan 30, 2017 • edited

Choose a reason for hiding this comment

fabpot commented Feb 12, 2017

robfrawley commented Jan 29, 2017 •

edited

ogizanagi Jan 29, 2017 •

edited

ogizanagi Jan 29, 2017 •

edited

robfrawley Jan 29, 2017 •

edited

robfrawley Jan 29, 2017 •

edited

robfrawley Jan 29, 2017 •

edited

nicolas-grekas Jan 30, 2017 •

edited

robfrawley Jan 30, 2017 •

edited

robfrawley Jan 30, 2017 •

edited