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

Improvement: Add Cache layer to Graph::build steps #18

Merged
merged 1 commit into from
Oct 18, 2021

Conversation

chrispenny
Copy link
Member

@chrispenny chrispenny commented Oct 10, 2021

Fixes #14
Closes #14

Using feature/many-many-support as the base so that I could test with "full support" added. Plus there were to method name changes on that branch.

Manual test

I made a quick test that ran each build method 100 times, and then output the average time per iteration in milliseconds. I set this up on a recent project that has a reasonably small configuration layer (8 blocks, 2 pages, Site Config, etc).

I was surprised by how quick the build process was (on average) even when uncached, but it's worth mentioning that it will (of course) get worse with scale.

Result

Uncached - milliseconds per build iteration: 22.61
Cached - milliseconds per build iteration: 0.63

Test that I ran

public function testPerformance(): void
{
    // Instantiate Graph. This should trigger a build
    $graph = Graph::singleton();

    // Use ReflectionClass to access private methods
    $reflectionClass = new ReflectionClass(Graph::class);
    $build = $reflectionClass->getMethod('buildEdges');
    $build->setAccessible(true);
    $create = $reflectionClass->getMethod('buildGlobalCares');
    $create->setAccessible(true);

    // Start the test
    $start = microtime(true);

    $testCount = 100;

    // Run our builds
    for ($i = 0; $i < $testCount; $i += 1) {
        $build->invoke($graph);
        $create->invoke($graph);
    }

    $end = microtime(true);
    $milliseconds = ($end - $start) / $testCount * 1000;
    // Average time per build
    Debug::dump(round($milliseconds, 2));
}

Test coverage

  • Test cache is available after build
  • Test Graph properties are set correctly from cache on build
  • Test Graph properties are set correctly from cache on secondary request

@chrispenny chrispenny force-pushed the feature/cache-graph branch 3 times, most recently from 3c80591 to 6c13f83 Compare October 11, 2021 00:19
@chrispenny chrispenny marked this pull request as ready for review October 11, 2021 00:32
Copy link
Contributor

@adrhumphreys adrhumphreys left a comment

Choose a reason for hiding this comment

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

Left a comment regarding the terminology but it's not essential before merging this in

@@ -1,10 +1,6 @@
---
Name: keysforcache
Name: kfc-blacklist
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to disallow list we should avoid using the terminology "blacklist"

@adrhumphreys adrhumphreys merged commit b2e0155 into feature/many-many-support Oct 18, 2021
@adrhumphreys adrhumphreys deleted the feature/cache-graph branch October 18, 2021 00:19
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.

2 participants