Skip to content

Performance take2 #69

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

Merged
merged 16 commits into from
Oct 7, 2013
Merged

Performance take2 #69

merged 16 commits into from
Oct 7, 2013

Conversation

slusarz
Copy link
Contributor

@slusarz slusarz commented Sep 25, 2013

So a smaller, less ambitious performance try. The combination of these provides about a 25% performance boost, at least according to xdebug data.

peek() is the deadly method that needs to be worked on further. It consumes about 36% of runtime. comes() is the next biggest performance hog, but it only consumes about 11%.

Especially when these functions are being called hundreds of thousands
of times during a parse, preincrementing absolutely is a noticeable
performance boost.
We can get this information from the peek() return.
We already do a boundary check later in the method (after determining
the peek size).
Only very rarely do we care about case sensitivity.  Most of the time,
we are dealing with symbols.  This saves on a ton of strtolower() calls.

Also can save on a more expensive mb_strlen() call if we know we are
dealing with symbols (for any practical charset).
Cache the common case of a single byte of peek data.
No reason for this method to be static
Optimize for the most-expected case - a non-prefixed identifier.
@sabberworm
Copy link
Collaborator

I don’t see much of an improvement in running time for the unit tests but it’s not getting worse either so I’ll merge this.

sabberworm added a commit that referenced this pull request Oct 7, 2013
Performance improvements by caching some invariants
@sabberworm sabberworm merged commit 1e5657b into MyIntervals:master Oct 7, 2013
@slusarz
Copy link
Contributor Author

slusarz commented Oct 7, 2013

Your unit tests are not a good real-world example, at least for run-times. Think of CSS parsing in the browser setting - you are parsing 100KB+ CSS stylesheets every few seconds.

Which is what we are seeing in our code (we might be seeing 3 different PHP processes concurrently parsing 200+ KB of CSS EVERY SECOND. This is not unexpected, since there are thousands of concurrent users). Thus, at least for us, parsing speed is an issue. And these changes half the parsing speed - as verified by xdebug - on larger CSS payloads.

@sabberworm
Copy link
Collaborator

I have some CSS test files locally (taken from Facebook and other live sites) that I haven’t committed into the repository as they don’t really test any features. I was talking about them. With your patch parsing all of these takes a second less than it did before (totalling 16 seconds).

@slusarz slusarz deleted the performance_take2 branch October 23, 2013 20:08
@slusarz
Copy link
Contributor Author

slusarz commented Oct 23, 2013

Will open another ticket with fix.

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.

2 participants