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

Fix address retrieval in cargo bench #138

Merged
merged 1 commit into from
Sep 9, 2019

Conversation

RReverser
Copy link
Contributor

@RReverser RReverser commented Sep 8, 2019

Fix address retrieval in cargo bench

First 200 lines from nm can contain irrelevant symbols which are all filtered out and benchmark starts testing no-op.

Change code to make sure that we always have at least 200 symbols after filtering out.

(I'm not sure if this was broken forever, or just depends on the distro / nm version.)

Before:

test context_new_and_query_location       ... bench:   3,483,195 ns/iter (+/- 176,722)
test context_new_and_query_with_functions ... bench:   3,500,515 ns/iter (+/- 171,316)
test context_new_location                 ... bench:   3,548,175 ns/iter (+/- 428,981)
test context_new_with_functions           ... bench:   3,935,070 ns/iter (+/- 1,562,079)
test context_query_location               ... bench:           0 ns/iter (+/- 0)
test context_query_with_functions         ... bench:           1 ns/iter (+/- 0)

After:

test context_new_and_query_location       ... bench:   6,120,630 ns/iter (+/- 1,106,154)
test context_new_and_query_with_functions ... bench:  37,054,340 ns/iter (+/- 4,360,871)
test context_new_location                 ... bench:   3,335,070 ns/iter (+/- 118,513)
test context_new_with_functions           ... bench:   3,335,590 ns/iter (+/- 362,814)
test context_query_location               ... bench:   1,196,900 ns/iter (+/- 252,891)
test context_query_with_functions         ... bench:   5,818,455 ns/iter (+/- 64,389)

Fixes #136.

@RReverser RReverser changed the title Make sure that we always capture 200 addresses Fix address retrieval in cargo bench Sep 8, 2019
@coveralls
Copy link

coveralls commented Sep 8, 2019

Coverage Status

Coverage remained the same at 73.669% when pulling 2969ce5 on RReverser:fix-cargo-bench into 24d669e on gimli-rs:master.

First 200 lines from nm can contain irrelevant symbols which are all filtered out and benchmark starts testing no-op.

Change code to make sure that we always have at least 200 symbols *after* filtering out.

(I'm not sure if this was broken forever, or just depends on the distro / nm version.)

Before:
```
test context_new_and_query_location       ... bench:   3,483,195 ns/iter (+/- 176,722)
test context_new_and_query_with_functions ... bench:   3,500,515 ns/iter (+/- 171,316)
test context_new_location                 ... bench:   3,548,175 ns/iter (+/- 428,981)
test context_new_with_functions           ... bench:   3,935,070 ns/iter (+/- 1,562,079)
test context_query_location               ... bench:           0 ns/iter (+/- 0)
test context_query_with_functions         ... bench:           1 ns/iter (+/- 0)
```

After:
```
test context_new_and_query_location       ... bench:   6,120,630 ns/iter (+/- 1,106,154)
test context_new_and_query_with_functions ... bench:  37,054,340 ns/iter (+/- 4,360,871)
test context_new_location                 ... bench:   3,335,070 ns/iter (+/- 118,513)
test context_new_with_functions           ... bench:   3,335,590 ns/iter (+/- 362,814)
test context_query_location               ... bench:   1,196,900 ns/iter (+/- 252,891)
test context_query_with_functions         ... bench:   5,818,455 ns/iter (+/- 64,389)
```

Fixes gimli-rs#136.
Copy link
Contributor

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greats, thanks!

@philipc philipc merged commit 5486ec7 into gimli-rs:master Sep 9, 2019
@philipc
Copy link
Contributor

philipc commented Sep 9, 2019

Looks like this depends on distro/nm version, I was getting 6320 entries in addresses. Something further to consider is it ensure whether we are getting enough addresses that are in inlined functions, especially in context_new_and_query_with_functions.

BTW, I currently have some in progress performance improvements that I don't have time to finish off right now. Let me know if you plan to do any work in this area so that we don't conflict.

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.

cargo bench is testing no-ops
3 participants