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 benchmarks with `OsIpcReceiver` not being `Sync` anymore #122

Merged
merged 1 commit into from Nov 20, 2016

Conversation

@antrik
Copy link
Contributor

antrik commented Nov 12, 2016

The first commit is the minimal fix to get it working again.

The second patch switches to a different thread handling approach, which inexplicably produces lower overhead in some (unrelated) cases on my system...

@antrik
Copy link
Contributor Author

antrik commented Nov 12, 2016

This supersedes #121

(Well, at least I think it's a preferable fix...)

Would be great if someone with a more modern system (or maybe even a different OS?) could check whether the second patch produces a visible difference -- especially in the threaded case (i.e. 256 k and above); though a visible difference for the non-threaded case would also be relevant to know...

@antrik antrik force-pushed the antrik:fix-bench branch from 9bdf0b8 to 9e60389 Nov 12, 2016
@vvuk
Copy link
Contributor

vvuk commented Nov 14, 2016

I just tested with and without this -- the big data benchmark numbers definitely change, e.g. from:

test size_19_512k   ... bench:     595,488 ns/iter (+/- 64,696)
test size_20_1m     ... bench:   1,107,073 ns/iter (+/- 151,017)
test size_21_2m     ... bench:   1,976,821 ns/iter (+/- 180,263)
test size_22_4m     ... bench:   3,737,198 ns/iter (+/- 438,126)
test size_23_8m     ... bench:   7,096,525 ns/iter (+/- 887,525)

to

test size_19_512k   ... bench:     558,607 ns/iter (+/- 22,566)
test size_20_1m     ... bench:   1,196,693 ns/iter (+/- 93,622)
test size_21_2m     ... bench:   2,102,265 ns/iter (+/- 230,279)
test size_22_4m     ... bench:   3,826,037 ns/iter (+/- 212,121)
test size_23_8m     ... bench:   7,267,728 ns/iter (+/- 852,904)

But not enough to care I don't think. We should still land this, as without it it's impossible to benchmark.. if someone wants to have more precise benchmarking they can work on that afterwards.

@antrik
Copy link
Contributor Author

antrik commented Nov 14, 2016

@vvuk can you check whether they change between applying only the first patch or both? That's what I'm mostly concerned about... (You could also compare against #121 for another variant.)

@vvuk
Copy link
Contributor

vvuk commented Nov 14, 2016

These are the latest Windows numbers, from my laptop (past thing was from a desktop). Second numbers look fine.

Before Sync change:

test create_channel ... bench:      14,904 ns/iter (+/- 1,757)
test size_00_1      ... bench:       1,671 ns/iter (+/- 136)
test size_01_2      ... bench:       1,667 ns/iter (+/- 137)
test size_02_4      ... bench:       1,665 ns/iter (+/- 186)
test size_03_8      ... bench:       1,664 ns/iter (+/- 165)
test size_04_16     ... bench:       1,708 ns/iter (+/- 211)
test size_05_32     ... bench:       1,643 ns/iter (+/- 114)
test size_06_64     ... bench:       1,681 ns/iter (+/- 166)
test size_07_128    ... bench:       1,691 ns/iter (+/- 143)
test size_08_256    ... bench:       1,715 ns/iter (+/- 164)
test size_09_512    ... bench:       1,842 ns/iter (+/- 237)
test size_10_1k     ... bench:       1,913 ns/iter (+/- 178)
test size_11_2k     ... bench:       2,065 ns/iter (+/- 108)
test size_12_4k     ... bench:       2,627 ns/iter (+/- 336)
test size_13_8k     ... bench:       3,155 ns/iter (+/- 235)
test size_14_16k    ... bench:      12,807 ns/iter (+/- 1,044)
test size_15_32k    ... bench:      16,779 ns/iter (+/- 2,277)
test size_16_64k    ... bench:      29,813 ns/iter (+/- 4,291)
test size_17_128k   ... bench:     338,392 ns/iter (+/- 105,054)
test size_18_256k   ... bench:     354,363 ns/iter (+/- 134,744)
test size_19_512k   ... bench:     463,017 ns/iter (+/- 179,602)
test size_20_1m     ... bench:     786,914 ns/iter (+/- 175,035)
test size_21_2m     ... bench:   1,254,104 ns/iter (+/- 470,615)
test size_22_4m     ... bench:   2,344,278 ns/iter (+/- 560,749)
test size_23_8m     ... bench:   4,597,099 ns/iter (+/- 739,417)

After first patch:

test create_channel ... bench:      14,603 ns/iter (+/- 6,303)
test size_00_1      ... bench:       1,639 ns/iter (+/- 186)
test size_01_2      ... bench:       1,639 ns/iter (+/- 150)
test size_02_4      ... bench:       1,633 ns/iter (+/- 165)
test size_03_8      ... bench:       1,633 ns/iter (+/- 185)
test size_04_16     ... bench:       1,601 ns/iter (+/- 135)
test size_05_32     ... bench:       1,580 ns/iter (+/- 164)
test size_06_64     ... bench:       1,650 ns/iter (+/- 204)
test size_07_128    ... bench:       1,626 ns/iter (+/- 119)
test size_08_256    ... bench:       1,676 ns/iter (+/- 161)
test size_09_512    ... bench:       1,795 ns/iter (+/- 190)
test size_10_1k     ... bench:       1,876 ns/iter (+/- 163)
test size_11_2k     ... bench:       2,023 ns/iter (+/- 188)
test size_12_4k     ... bench:       2,581 ns/iter (+/- 192)
test size_13_8k     ... bench:       3,277 ns/iter (+/- 251)
test size_14_16k    ... bench:      17,573 ns/iter (+/- 3,355)
test size_15_32k    ... bench:      18,197 ns/iter (+/- 3,273)
test size_16_64k    ... bench:      30,316 ns/iter (+/- 3,435)
test size_17_128k   ... bench:     315,150 ns/iter (+/- 37,126)
test size_18_256k   ... bench:     341,686 ns/iter (+/- 101,133)
test size_19_512k   ... bench:     444,938 ns/iter (+/- 117,830)
test size_20_1m     ... bench:     770,825 ns/iter (+/- 113,292)
test size_21_2m     ... bench:   1,243,743 ns/iter (+/- 185,992)
test size_22_4m     ... bench:   2,303,507 ns/iter (+/- 430,200)
test size_23_8m     ... bench:   4,457,642 ns/iter (+/- 533,357)

After second patch:

test create_channel ... bench:      15,004 ns/iter (+/- 1,947)
test size_00_1      ... bench:       1,642 ns/iter (+/- 149)
test size_01_2      ... bench:       1,642 ns/iter (+/- 135)
test size_02_4      ... bench:       1,640 ns/iter (+/- 130)
test size_03_8      ... bench:       1,638 ns/iter (+/- 242)
test size_04_16     ... bench:       1,628 ns/iter (+/- 117)
test size_05_32     ... bench:       1,526 ns/iter (+/- 600)
test size_06_64     ... bench:       1,631 ns/iter (+/- 140)
test size_07_128    ... bench:       1,552 ns/iter (+/- 464)
test size_08_256    ... bench:       1,673 ns/iter (+/- 164)
test size_09_512    ... bench:       1,788 ns/iter (+/- 186)
test size_10_1k     ... bench:       1,853 ns/iter (+/- 193)
test size_11_2k     ... bench:       2,016 ns/iter (+/- 207)
test size_12_4k     ... bench:       2,567 ns/iter (+/- 241)
test size_13_8k     ... bench:       3,259 ns/iter (+/- 235)
test size_14_16k    ... bench:      13,542 ns/iter (+/- 1,424)
test size_15_32k    ... bench:      17,592 ns/iter (+/- 2,304)
test size_16_64k    ... bench:      29,932 ns/iter (+/- 2,842)
test size_17_128k   ... bench:     276,133 ns/iter (+/- 53,773)
test size_18_256k   ... bench:     309,478 ns/iter (+/- 56,093)
test size_19_512k   ... bench:     381,911 ns/iter (+/- 98,733)
test size_20_1m     ... bench:     793,852 ns/iter (+/- 133,922)
test size_21_2m     ... bench:   1,216,504 ns/iter (+/- 251,077)
test size_22_4m     ... bench:   2,325,117 ns/iter (+/- 375,545)
test size_23_8m     ... bench:   4,535,233 ns/iter (+/- 764,413)
@antrik
Copy link
Contributor Author

antrik commented Nov 15, 2016

So just like on Linux, there is a lot of fluctuation in the threaded tests.

I suspect the lower numbers for 128k and 256k in the last run are also just a random fluctuation rather than actually better results?

At any rate, it doesn't look like either variant actually makes things worse :-)

@antrik antrik force-pushed the antrik:fix-bench branch from 9e60389 to b3a585d Nov 16, 2016
@antrik
Copy link
Contributor Author

antrik commented Nov 16, 2016

I decided to drop the second patch, as it's really an unrelated change -- so this is just a trivial fix now.

@antrik antrik force-pushed the antrik:fix-bench branch from b3a585d to 44894df Nov 18, 2016
@wafflespeanut
Copy link
Member

wafflespeanut commented Nov 20, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2016

📌 Commit 44894df has been approved by Wafflespeanut

@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2016

Testing commit 44894df with merge 1c12453...

bors-servo added a commit that referenced this pull request Nov 20, 2016
Fix benchmarks with `OsIpcReceiver` not being `Sync` anymore

The first commit is the minimal fix to get it working again.

The second patch switches to a different thread handling approach, which inexplicably produces lower overhead in some (unrelated) cases on my system...
@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2016

☀️ Test successful - status-appveyor, status-travis

@bors-servo bors-servo merged commit 44894df into servo:master Nov 20, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.