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

Improvements for loader speeds #841

Closed

Conversation

dlsniper
Copy link
Contributor

This is something started from #822 .

It attempts to improve the speed of the chain loader and rest of the standard loaders by adding a cache level and a hasSource() method for them in order gain speed.

@@ -194,4 +199,8 @@ protected function guessTemplateInfo()
}
}
}

public static function setExtendedErrorHandling($mode) {
Copy link
Member

Choose a reason for hiding this comment

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

please put the curly brace on its own line

@dlsniper
Copy link
Contributor Author

Thanks for the feedback, I'll do the changes tonight or during the coming weekend.

As for the utility of the ::setExtendedErrorHandling() function, this saves me about 25% to 40% on the benchkmark Fabien presented in the original ticket.

Thanks!

*/

/**
* Interface all loaders must implement.
Copy link
Contributor

Choose a reason for hiding this comment

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

probably not true

@dlsniper
Copy link
Contributor Author

I've added some caching for the Chain Loader but it's still a WIP. I'll work more on this in the coming hours.

@dlsniper
Copy link
Contributor Author

I'm getting better values right now that I had before, standing on:

0.09
0.07 ... x 1
0.07 ... x 1

I've also noticed that if I run the tests from the browser then I'll have an overall slowdown of about 0.10 which I'm not sure from where it comes from. Any pointers before me digging a bit more into Twigs internals.

@dlsniper
Copy link
Contributor Author

dlsniper commented Oct 3, 2012

@fabpot what's your opinion about this PR?

foreach ($this->loaders as $loader) {
if ($loader instanceof Twig_AdvancedLoaderInterface && $loader->hasSource($name)) {
$this->hasSourceCache[$name] = true;
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

how about return $this->hasSourceCache[$name] = true;

@dlsniper
Copy link
Contributor Author

@Tobion and @fabpot done and done :)

I'm not sure why the tests are failing on PHP 5.4 as I don't have it installed anywhere.

@@ -77,6 +77,10 @@ public function setPaths($paths, $namespace = '__main__')
*
* @param string $path A path where to look for templates
* @param string $namespace A path name
*
* @return void
Copy link
Contributor

Choose a reason for hiding this comment

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

All @return void should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any PSR rule that I'm missing here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what we do everywhere else.

*
* @param string $name The name of the template to check if we can load
*
* @return boolean If the template source code in handled by this loader or not
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

@fabpot fabpot closed this in ed05546 Oct 19, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants