Closed vinniefalco closed 8 years ago
@@ master #35 diff @@
==========================================
Files 27 28 +1
Lines 1719 1710 -9
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 1639 1656 +17
+ Misses 80 54 -26
Partials 0 0
Powered by Codecov. Last update 4bb090c...c062611
Reviewed 53 of 54 files at r1, 5 of 5 files at r2, 1 of 1 files at r3. Review status: all files reviewed at latest revision, 7 unresolved discussions.
bench/bench.cpp, line 142 [r3] (raw file):
Fetcher&& fetcher, AddSample&& add_sample, PreFetchHook&& pre_fetch_hook,
I don't know enough about this, but I wonder if pre_fetch_hook is always useful/needed - if not, could a default noop() be provided here?
bench/bench.cpp, line 427 [r3] (raw file):
bench_progress progress(derr, total_ops); enum
do we prefer enum class
?
bench/bench.cpp, line 445 [r3] (raw file):
// Reserve up front to database that run later don't have less memory for (int i = 0; i < db_last; ++i) for (int j = 0; i < db_last; ++i)
should these be all j
s here - or are you doing something tricky that I'm missing? If it is the simple inner index, then I guess compare against op_last
would make sense too..
bench/bench.cpp, line 482 [r3] (raw file):
auto const iter_w = 15; for(int op_idx=0;op_idx<op_last;++op_idx)
minor nit: space after semicolons?
bench/bench.cpp, line 504 [r3] (raw file):
auto const min_sample = [&ops_per_sec]{ for (auto i = 0; i < db_last; ++i) if (!ops_per_sec[i][op_fetch].empty())
it's not obvious to me why min_sample
only considers op_fetch
while max_sample
considers the full matrix.
include/nudb/impl/rekey.ipp, line 41 [r3] (raw file):
{ static_assert(is_File<File>::value, "File requiremnts not met");
typo - probably search/replace this particular typo
[include/nudb/impl/verify.ipp, line 43 [r3]](https://reviewable.io:443/reviews/vinniefalco/nudb/35#-KRAbbDxrxrlO-ekecT:-KRAbbDxrxrlO-ekecTa:b-c35zko) (raw file):_
{ static_assert(is_File<File>::value, "File requiremnts not met");
typo (requirements)
Comments from Reviewable
Review status: all files reviewed at latest revision, 7 unresolved discussions.
Comments from Reviewable
@mellery451 ugh..
Review status: 54 of 59 files reviewed at latest revision, 7 unresolved discussions.
bench/bench.cpp, line 427 [r3] (raw file):
For an in-function enum, in a benchmark program, not part of public API... I don't think I'm going to get too worked up about it.
Comments from Reviewable
Review status: 54 of 59 files reviewed at latest revision, 6 unresolved discussions.
bench/bench.cpp, line 445 [r3] (raw file):
Looks like a bug
include/nudb/impl/rekey.ipp, line 41 [r3] (raw file):
Done.
[include/nudb/impl/verify.ipp, line 43 [r3]](https://reviewable.io:443/reviews/vinniefalco/nudb/35#-KRAbbDxrxrlO-ekecT:-KRAkDBoVxvuWo3ZtyD7:b-896fix) (raw file):_
Done.
Comments from Reviewable
Review status: 54 of 59 files reviewed at latest revision, 5 unresolved discussions.
bench/bench.cpp, line 142 [r3] (raw file):
It's hard to write a default here because
PreFetchHook
is a template and lambdas have unutterable types. If we make this astd::function
I could add a default, but since this is a self-contained file I prefer to just leave it as a lambda.
bench/bench.cpp, line 445 [r3] (raw file):
That's a copy/paste bug. Though it won't effect results, it just didn't reserve the space for all the indexes. Fixed.
bench/bench.cpp, line 482 [r3] (raw file):
Fixed
bench/bench.cpp, line 504 [r3] (raw file):
Inserts always start with a database with zero items, I skip that because
fetch
will not have a sample at zero. However, this can be simplified to just use the batch_size. Fixed.
Comments from Reviewable
Reviewed 6 of 6 files at r4. Review status: all files reviewed at latest revision, 4 unresolved discussions.
Comments from Reviewable
Reviewed 48 of 54 files at r1, 5 of 5 files at r2, 1 of 1 files at r3, 6 of 6 files at r4, 1 of 1 files at r5. Review status: 55 of 59 files reviewed at latest revision, 8 unresolved discussions.
bench/bench.cpp, line 258 [r3] (raw file):
auto pre_fetch_hook = [&ts, &ec]() { // Open the close the db so the commit thread does not confound the // timings
Typo in the comment? Should it read Close then open the db
?
doc/master.qbk, line 215 [r3] (raw file):
The application-defined constant is a 64-bit unsigned integer which the caller may set to any value. This value can be retrieved on an open database, where it wil lbe equal to the value used at creation time. This constant can
typo, wil lbe
include/nudb/verify.hpp, line 31 [r5] (raw file):
@li @b 1 Fast algorith */ int algorithm; // 0 = normal, 1 = fast
Make an enum?
test/buffer.cpp, line 25 [r5] (raw file):
using buffer = nudb::detail::buffer; static_assert(std::is_default_constructible<buffer>::value, ""); #if 0
Buffer requirements are changing?
Comments from Reviewable
Reviewed 4 of 4 files at r6. Review status: all files reviewed at latest revision, 8 unresolved discussions.
Comments from Reviewable
Review status: 56 of 59 files reviewed at latest revision, 21 unresolved discussions.
bench/bench.cpp, line 507 [r5] (raw file):
result_dict const& dict, std::uint64_t key) { dout << std::setw(col_w) << std::fixed << std::setprecision(2); // Take the average of all the values, or "NA"" if none collected
Minor nit: redundant double quote after NA
bench/bench.cpp, line 521 [r5] (raw file):
} }; for (std::uint64_t n = 0; n <= max_sample; n = n ? 10 * n : 10)
This ternary is really ugly and I'm surprised it doesn't spew warnings. Is there a point to testing with n == 0
? Why not start with 1
instead?
doc/master.qbk, line 214 [r5] (raw file):
The application-defined constant is a 64-bit unsigned integer which the caller may set to any value. This value can be retrieved on an open database,
"retrieved on an open" -> "retrieved from an open"
_include/nudb/basic_store.hpp, line 77 [r5] (raw file):_
std::size_t rate = 0; time_point when = clock_type::now();
Rogue white space?
_include/nudb/basic_store.hpp, line 156 [r5] (raw file):_
@par Requirements
Rogue white space?
_include/nudb/basic_store.hpp, line 160 [r5] (raw file):_
@par Thread safety
Rogue white space?
_include/nudb/posix_file.hpp, line 117 [r5] (raw file):_
/** Remove a file from the file system. It is not an error to attempt erasing a file that does not exist.
"to attempt erasing" -> "to attempt to erase"
_include/nudb/win32_file.hpp, line 135 [r5] (raw file):_
/** Remove a file from the file system. It is not an error to attempt erasing a file that does not exist.
"to attempt erasing" -> "to attempt to erase"
include/nudb/detail/arena.hpp, line 136 [r5] (raw file):
return nullptr; auto const p = const_cast<std::uint8_t*>( reinterpret_cast<uint8_t const*>(this + 1)
Why do the const_cast
dance here? It doesn't seem needed: alloc
isn't const
so, this
isn't either. Why not simply: auto p = reinterpret_cast<uint8_t*>(this + 1) + used_;
include/nudb/detail/cache.hpp, line 80 [r5] (raw file):
// Constructs a cache that will never have inserts cache_t() = default;
Rogue white space
_include/nudb/impl/basic_store.ipp, line 18 [r5] (raw file):_ This is strictly unnecessary: per §16.1 ¶4 of the C++ standard:
After all replacements due to macro expansion and the defined unary operator have been performed, all remaining identifiers and keywords, except for
true
andfalse
, are replaced with the pp-number 0
_include/nudb/impl/basic_store.ipp, line 568 [r5] (raw file):_
cache c0(s_->kh.key_size, s_->kh.block_size, "c0"); cache c1(s_->kh.key_size, s_->kh.block_size, "c1"); // 0.63212 ~= 1 - 1/e
Interesting fact. Cryptic comment.
_include/nudb/impl/basic_store.ipp, line 668 [r5] (raw file):_
// This might be slightly better than the old // view since there could be fewer spills. m.lock();
FYI: If any of the operations below throw
then m
will stay locked. I haven't verified whether they do or not.
Comments from Reviewable
Reviewed 47 of 54 files at r1, 4 of 5 files at r2, 1 of 1 files at r3, 6 of 6 files at r4, 1 of 1 files at r5. Review status: 56 of 59 files reviewed at latest revision, 21 unresolved discussions.
Comments from Reviewable
Review status: 56 of 59 files reviewed at latest revision, 21 unresolved discussions.
_bench/bench.cpp, line 521 [r5] (raw file):_
I used to report the timing for inserting the initial batch into the zero-size database. When I took that out I missed changing the loop bounds. Fixed.
Comments from Reviewable
Review status: 55 of 59 files reviewed at latest revision, 21 unresolved discussions.
bench/bench.cpp, line 521 [r5] (raw file):
I notice that
n
now starts from100
. I suspect that's what you want, but just want to make sure.
Comments from Reviewable
Review status: 55 of 59 files reviewed at latest revision, 20 unresolved discussions.
_include/nudb/basic_store.hpp, line 77 [r5] (raw file):_
No idea what you're going on about
include/nudb/verify.hpp, line 31 [r5] (raw file):
I could make it an
enum
but I don't want to draw that much attention to this member.
include/nudb/detail/arena.hpp, line 136 [r5] (raw file):
It was required to compile on all our targets. Maybe that's changed. But I don't know that I want to risk breaking it.
_include/nudb/impl/basic_store.ipp, line 18 [r5] (raw file):_
That is true, but this define serves two purposes: to illustrate to the reader that its meant to be changed, and to provide a place where someone can edit the value and easily make it 1 or 0 without deleting the line.
_include/nudb/impl/basic_store.ipp, line 568 [r5] (raw file):_
I'm open to suggested wording but I'm not sure that it matters. The exact number is not so important, only that it is less than one and more than 0.5.
_include/nudb/impl/basic_store.ipp, line 668 [r5] (raw file):_
That's fine, nothing is allowed to throw. Even so, the caller provides
m
which goes out of scope on a stack unwinding so we're good.
test/buffer.cpp, line 25 [r5] (raw file):
Eventually I would like to elevate
buffer
to a public interface. This commented out code serves as a reminder.
Comments from Reviewable
OK
Reviewed 1 of 1 files at r7, 4 of 4 files at r8, 3 of 3 files at r9. Review status: all files reviewed at latest revision, 16 unresolved discussions.
_Comments from Reviewable_
Review status: 58 of 59 files reviewed at latest revision, 16 unresolved discussions.
_bench/bench.cpp, line 258 at r3 (raw file):_
Done.
Comments from Reviewable
Review status: 58 of 59 files reviewed at latest revision, 16 unresolved discussions.
_include/nudb/posix_file.hpp, line 117 at r5 (raw file):_
Done.
_include/nudb/verify.hpp, line 31 at r5 (raw file):_
Done.
Comments from Reviewable
All of the current comments have been addressed, is this ready to merge?
Review status: 58 of 59 files reviewed at latest revision, 10 unresolved discussions.
doc/master.qbk, line 215 at r3 (raw file):
Done.
Comments from Reviewable
:neckbeard::+1:
This change is
Reviewers: @mellery451 @JoelKatz @bachase
HTML docs link: http://vinniefalco.github.io/nudb