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 cgo argument check panic for small slices from bytes.Buffer (and similar types) #57

Merged
merged 5 commits into from
Mar 24, 2016

Conversation

bmatsuo
Copy link
Owner

@bmatsuo bmatsuo commented Mar 8, 2016

Fixes #56

The valBytes() function has been modified to return a non-empty slice of bytes for any (possibly empty) input slice. The length returned is always the true length of the input.

This is a hack that allows the cgo tool to correctly identify the argument type as the slice and not the bytes.Buffer (or whatever containing type causes these types of panics).

Small amounts of data written from bytes.Buffer can trigger a panic
during cgo argument checking because the pointer points inside the
Buffer struct.

This commit ensures that byte addressing expressions appear in the
arguments to C calls to avoid confusing cgo.  This appears to have
negative effects on benchmark times.  The -test.benchmem flag seems to
indicate that extra allocations are occurring
This saves passing a few unnecessary pointers, passing nil instead.
This should speed up the cgo check by a small amount.

There seems to be a small speed up from this.
@bmatsuo
Copy link
Owner Author

bmatsuo commented Mar 8, 2016

These are the current benchmark results, compared against master. There seems to be a fairly consistent penalty of 100ns+ for the relevant benchmarks which causes a slowdown of 7-43% depending on the benchmark.

Note: both benchmarks use go1.6 so and the baseline cgo argument check penalty affects both benchmarked versions.

benchmark                              old ns/op     new ns/op     delta
BenchmarkEnv_ReaderList-4              51685         51360         -0.63%
BenchmarkTxn_Put-4                     1519          1764          +16.13%
BenchmarkTxn_PutReserve-4              1614          1739          +7.74%
BenchmarkTxn_PutReserve_writemap-4     1351          1456          +7.77%
BenchmarkTxn_Put_writemap-4            1288          1516          +17.70%
BenchmarkTxn_Get_ro-4                  1385          1514          +9.31%
BenchmarkTxn_Get_raw_ro-4              778           891           +14.52%
BenchmarkScan_ro-4                     5912178       5907229       -0.08%
BenchmarkScan_raw_ro-4                 1507978       1527135       +1.27%
BenchmarkCursor-4                      692           680           -1.73%
BenchmarkCursor_Renew-4                178           181           +1.69%
BenchmarkTxn_Sub_commit-4              195340        188901        -3.30%
BenchmarkTxn_Sub_abort-4               184989        185890        +0.49%
BenchmarkTxn_abort-4                   14432         14530         +0.68%
BenchmarkTxn_commit-4                  14077         14346         +1.91%
BenchmarkTxn_ro-4                      145083        140361        -3.25%
BenchmarkTxn_unmanaged_abort-4         14481         13970         -3.53%
BenchmarkTxn_unmanaged_commit-4        14473         14238         -1.62%
BenchmarkTxn_unmanaged_ro-4            145395        153063        +5.27%
BenchmarkTxn_renew-4                   528           536           +1.52%
BenchmarkTxn_Put_append-4              477           686           +43.82%
BenchmarkTxn_Put_append_noflag-4       618           854           +38.19%

@bmatsuo
Copy link
Owner Author

bmatsuo commented Mar 23, 2016

It seems what performance loses remain here will not be regained until (possibly) go1.7. That is unfortunate. But if nothing else maybe SSA can smooth it all over.

But, I think I need to just bite the bullet and merge this now =\ The possibility of runtime panic is too real and hard to predict/handle.

@mranney
Copy link

mranney commented Mar 23, 2016

That's annoying to lose some perf, but I support your decision to favor stability. :)

@bmatsuo
Copy link
Owner Author

bmatsuo commented Mar 23, 2016

Indeed. At least I don't think the slowdown is a killer -- 100ns per OP. It should be OK for the time being. LMDB should still handily outperform BoltDB with these changes that gulf in performance is pretty large.

@bmatsuo bmatsuo merged commit cd9b251 into master Mar 24, 2016
@bmatsuo bmatsuo deleted the bmatsuo/fix-cgo-arguments branch March 24, 2016 20:11
@bmatsuo bmatsuo mentioned this pull request Mar 31, 2016
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