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

Faster performance when processing the clang AST #236

Merged
merged 2 commits into from Oct 17, 2017
Merged

Faster performance when processing the clang AST #236

merged 2 commits into from Oct 17, 2017

Conversation

Konstantin8105
Copy link
Contributor

@Konstantin8105 Konstantin8105 commented Oct 14, 2017

I use 2 ways for change the transpiling perfomance:

  1. Changes inside ast.
    Before: for execute parseBuiltinType we compile the Regex inside function groupsFromRegex. So, if we have N-elements of BuiltinType - we create N-elements of Regex.
    After : Now, we create only one Regex for BuiltinType. So, we save the time to compile the Regex.
    That changes created not for all AST elements

  2. Change inside main.go, see convertLinesToNodes. We can save the time, because at the begin we know "Amount of nodes is not more amount of Lines".

Results:
go install ; time $GOPATH/bin/c2go transpile -o /tmp/for.go ./tests/for.c
Before:

real	0m3.144s
user	0m3.496s
sys	0m0.272s

After

real	0m2.816s
user	0m3.132s
sys	0m0.288s

Run go test -tags=integration ./...:
Before:

ok      github.com/elliotchance/c2go    360.198s

After:

ok      github.com/elliotchance/c2go    339.361s

Little bit faster at 6...10%.

Please clarify your point of view - Is it interesting for project?


This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 14, 2017

Codecov Report

Merging #236 into master will increase coverage by 0.01%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
+ Coverage   80.82%   80.83%   +0.01%     
==========================================
  Files         134      134              
  Lines        5313     5318       +5     
==========================================
+ Hits         4294     4299       +5     
  Misses        832      832              
  Partials      187      187
Impacted Files Coverage Δ
main.go 52.66% <100%> (+0.85%) ⬆️
ast/ast.go 97.4% <83.33%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efb920d...1a28328. Read the comment docs.

@Konstantin8105
Copy link
Contributor Author

Just for information:
Before
In according to that line:
https://codecov.io/gh/elliotchance/c2go/src/efb920dbbee77cc0418b48d93a551b44d09c7289/ast/ast.go#L219
function run >10K times and compile >10K Regex objects
That time we can optimize.

@elliotchance
Copy link
Owner

Review status: 0 of 7 files reviewed at latest revision, 1 unresolved discussion.


ast/ast.go, line 225 at r1 (raw file):

	fullRegexp := "(?P<address>[0-9a-fx]+) " +
		strings.Replace(strings.Replace(rx, "\n", "", -1), "\t", "", -1)
	re := regexp.MustCompile(fullRegexp)

I see what this PR is trying to do and its actually a very good one! You are right, this wasn't a performance problem until we introduced the SQLite3 source which is huge.

This line is where you want to put the caching (rather than the other changes):

var cachedRegex := map[string]*Regexp{}

func groupsFromRegex(rx, line string) map[string]string {
    // ...

    if _, ok := cachedRegex[fullRegexp]; !ok {
        cachedRegex[fullRegexp] = regexp.MustCompile(fullRegexp)
    }
    re := cachedRegex[fullRegexp]

    // ...
}

Comments from Reviewable

@elliotchance
Copy link
Owner

Review status: 0 of 7 files reviewed at latest revision, 2 unresolved discussions.


a discussion (no related file):
This will be a great PR. I have left a comment on a smaller change to make. Please try that (discarding other changes) and see how much the performance changes 👍


Comments from Reviewable

@Konstantin8105
Copy link
Contributor Author

Review status: 0 of 7 files reviewed at latest revision, 2 unresolved discussions.


a discussion (no related file):

Previously, elliotchance (Elliot Chance) wrote…

This will be a great PR. I have left a comment on a smaller change to make. Please try that (discarding other changes) and see how much the performance changes 👍

Ok.


ast/ast.go, line 225 at r1 (raw file):

Previously, elliotchance (Elliot Chance) wrote…

I see what this PR is trying to do and its actually a very good one! You are right, this wasn't a performance problem until we introduced the SQLite3 source which is huge.

This line is where you want to put the caching (rather than the other changes):

var cachedRegex := map[string]*Regexp{}

func groupsFromRegex(rx, line string) map[string]string {
    // ...

    if _, ok := cachedRegex[fullRegexp]; !ok {
        cachedRegex[fullRegexp] = regexp.MustCompile(fullRegexp)
    }
    re := cachedRegex[fullRegexp]

    // ...
}

That the reason - Why I don`t create a full design. Your design with cache is awesome.


Comments from Reviewable

@elliotchance
Copy link
Owner

Reviewed 1 of 7 files at r1, 1 of 6 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@elliotchance elliotchance changed the title Performance test Faster performance when processing the clang AST Oct 17, 2017
@elliotchance elliotchance merged commit 42a73ea into elliotchance:master Oct 17, 2017
@elliotchance
Copy link
Owner

@Konstantin8105 Konstantin8105 deleted the experiment1 branch October 20, 2017 23:04
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