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

Evaluate ExcludeIf after JMS exclusion strategy #212

Merged

Conversation

romainneutron
Copy link
Contributor

This change enhance the perf of the ExclusionManager.

Most of all JMS ExclusionStrategyInterface relies on counter, isset and other basic language functionalities, while the evaluator allows to instantiate service or triggers voter through isGranted.

Evaluating this code whereas simple checks like groups or version already exclude adds a significant overhead.

This change inverts the order of the execution.

This change enhance the perf of the ExclusionManager.

Most of all JMS ExclusionStrategyInterface relies on counter, isset and other basic language functionalities, while the evaluator allows to instantiate service or triggers voter through isGranted.

Evaluating this code whereas simple checks like groups or version already exclude adds a significant overhead.

This change inverts the order of the execution.
@adrienbrault
Copy link
Contributor

Looks good! 👍

willdurand added a commit that referenced this pull request Sep 9, 2015
Evaluate ExcludeIf after JMS exclusion strategy
@willdurand willdurand merged commit 669eba5 into willdurand:master Sep 9, 2015
@willdurand
Copy link
Owner

thanks

@romainneutron romainneutron deleted the evaluate-exclude-if-after branch September 9, 2015 08:18
@romainneutron
Copy link
Contributor Author

Thanks for this fast merge!

@willdurand
Copy link
Owner

Need a new version?

@lyrixx
Copy link
Contributor

lyrixx commented Sep 9, 2015

Yes please ;)

@willdurand
Copy link
Owner

Here it is: 2.8.0!

@lyrixx
Copy link
Contributor

lyrixx commented Sep 9, 2015

Thanks ;)

@romainneutron
Copy link
Contributor Author

👯

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.

4 participants