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

[WIP] Optimize all matchers for performance #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thiemowmde
Copy link
Contributor

No description provided.

@thiemowmde thiemowmde force-pushed the denormalization branch 3 times, most recently from 54ffb1d to eae674b Compare April 18, 2018 13:15
@manicki
Copy link
Member

manicki commented Apr 24, 2018

Out of curiosity: as this mentions performance, do you have any numbers or so regarding the performance?

@thiemowmde
Copy link
Contributor Author

I rebased this after #12 got merged.

Unfortunately the performance gain is much smaller than I was hoping for.

You see, what this patch does is adding shortcuts to code paths doing a trivial string equality check. Instead of wrapping strings in IsEqual matchers, the string is kept. Most notably this allows to use DOMElement::getElementsByTagName, which is a hash lookup. Before this code path always iterated all child elements in a naive loop.

Unfortunately the new code path can only be hit when the name of the child element is provided as a string, e.g. havingChild( 'input' ). But this is not what is done in our tests. Instead all relevant tests do something like this: havingChild( both( withTagName( 'input' ) )->andAlso( withAttribute( …. To optimize this I would need to make changes to both, which is part of the upstream Hamcrest library. :-(

An alternative approach is to introduce something like havingTag( $tagName )->withAttribute( …, which can be used without both.

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

2 participants