bmatsuo / lmdb-go

Bindings for the LMDB C library
BSD 3-Clause "New" or "Revised" License
157 stars 59 forks source link

Fix cgo argument check panic for small slices from bytes.Buffer (and similar types) #57

Closed bmatsuo closed 8 years ago

bmatsuo commented 8 years ago

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).

bmatsuo commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

bmatsuo commented 8 years ago

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.