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

Performance suggestion: reduce allocations #21

Closed
YuriyNasretdinov opened this issue May 30, 2018 · 6 comments
Closed

Performance suggestion: reduce allocations #21

YuriyNasretdinov opened this issue May 30, 2018 · 6 comments

Comments

@YuriyNasretdinov
Copy link

The parser is not as performant as it could be (PHP7+ also creates AST in order to execute file):

$ php -n -r '$start = microtime(true); require("some_big_file.php"); echo "time: " . (microtime(true) - $start) . " sec\n";'
time: 0.0185 sec

$ go run test.go some_big_file.php
Errors count:  0
Parser time: 167.636796ms

test.go.zip

I made a few (very hacky) patches that significantly reduce allocations count for some critical parts and it reduced parsing time by a factor of 1.5:

# after patches
$ go run test.go some_big_file.php
Errors count:  0
Parser time: 109.522377ms

There are still plenty more allocations that profiler shows, my patches are just proof-of-concept.
speedup.patch.txt

@YuriyNasretdinov
Copy link
Author

Performance is important because even for a pretty complex linter parsing consumes about 80% of overall time which is not really what I would like to get :).

@z7zmey
Copy link
Owner

z7zmey commented May 31, 2018

position/builder.go
scanner/lexer.go
token/token.go

Changes in that files reduce memory allocations in five times, especially scanner/lexer.go. Сhanges in other files have less effect.
Can you create a pull request?

Also, I see you disable saving comments, I have created issue #22, it increases speed when comments unnecessary.

@YuriyNasretdinov
Copy link
Author

YuriyNasretdinov commented May 31, 2018

There are a lot of allocations done in strings.TrimLeft as well as it creates new temporary function every time: https://golang.org/src/strings/strings.go?s=20503:20548#L785

You can use strings.TrimPrefix though, it probably will be faster too:

https://golang.org/src/strings/strings.go?s=21232:21272#L809

@YuriyNasretdinov
Copy link
Author

YuriyNasretdinov commented May 31, 2018

I really want to have some generic approach of grouping allocations for all AST nodes, not just for most popular ones (as I did). It will give huge performance boost and (probably) reduce consumed memory too.

I am not sure how to implement it properly in this project so I just created this issue to discuss possible solutions if you are interested :).

@z7zmey
Copy link
Owner

z7zmey commented Jun 4, 2018

788628d reduces allocations from 10405 to 7430 in benchmark and reduces cpu usage by bytes.Buffer.WriteByte

Before:
2018-06-05 02 40 21

After:
2018-06-05 02 44 36

z7zmey added a commit that referenced this issue Jun 4, 2018
#21 scanner.Lexer.charsToBytes optimization
@z7zmey
Copy link
Owner

z7zmey commented Jun 5, 2018

By using pprof, I have found a number of bottlenecks and created issues for them.

@z7zmey z7zmey closed this as completed Jun 5, 2018
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

No branches or pull requests

2 participants