Summary:
scan-build is reporting two memory leak bugs in `table/block_based_table_reader.cc`. They are both false positives. In both cases we allocate memory in `ReadBlockFromFile` if `s.ok()`. Then after the function `ReadBlockFromFile` returns we check for the same variable if `s.ok()` and then use the memory that was allocated. The bugs reported by scan-build is if `ReadBlockFromFile` allocates memory and returns, but for some reason status `s` is not the same and `s.ok() != true`.
In this case scan-build is concerned that memory owner transfer is not explicit. I modified `ReadBlockFromFile` to accept `std::unique_ptr<Block>*` as a parameter, instead of raw pointer.
scan-build reports:
http://home.fburl.com/~sugak/latest2/report-a4b3fa.html#EndPathhttp://home.fburl.com/~sugak/latest2/report-29adbf.html#EndPath
Test Plan:
Make sure scan-build does not report these bugs and all tests are passing.
```lang=bash
% make check
% make analyze
```
Reviewers: sdong, lgalanis, meyering, igor
Reviewed By: igor
Subscribers: dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D33681
Summary:
Prior to this change, "make check" would always waste a lot of
time relinking 60+ binaries. With this change, it does that
only when the generated file, util/build_version.cc, changes,
and that happens only when the date changes or when the
current git SHA changes.
This change makes some other improvements: before, there was no
rule to build a deleted util/build_version.cc. If it was somehow
removed, any attempt to link a program would fail.
There is no longer any need for the separate file,
build_tools/build_detect_version. Its functionality is
now in the Makefile.
* Makefile (DEPFILES): Don't filter-out util/build_version.cc.
No need, and besides, removing that dependency was wrong.
(date, git_sha, gen_build_version): New helper variables.
(util/build_version.cc): New rule, to create this file
and update it only if it would contain new information.
* build_tools/build_detect_platform: Remove file.
* db/db_impl.cc: Now, print only date (not the time).
* util/build_version.h (rocksdb_build_compile_time): Remove
declaration. No longer used.
Test Plan:
- Run "make check" twice, and note that the second time no linking is performed.
- Remove util/build_version.cc and ensure that any "make"
command regenerates it before doing anything else.
- Run this: strings librocksdb.a|grep _build_.
That prints output including the following:
rocksdb_build_git_date:2015-02-19
rocksdb_build_git_sha:2.8.fb-1792-g3cb6cc0
Reviewers: ljin, sdong, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33591
Summary: Add a DB property about live versions. It can be helpful to figure out whether there are files not live but not yet deleted, in some use cases.
Test Plan: make all check
Reviewers: rven, yhchiang, igor
Reviewed By: igor
Subscribers: yoshinorim, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D33327
Summary:
The "ar" command was excessively verbose.
This abbreviates it to be like CC and LD.
Also, factor "rs" options into ARFLAGS.
Test Plan:
$ rm librocksdb.a
$ make librocksdb.a
AR librocksdb.a
ar: creating librocksdb.a
Reviewers: ljin, sdong, igor.sugak, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33699
Summary: Replaced rapidjson with fbson
Test Plan:
make all check
make valgrind_check
Reviewers: golovachalexander, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D32733
Summary:
This is a diff for managed iterator. A managed iterator
is a wrapper around an iterator which saves the options for that
iterator as well as the current key/value so that the underlying iterator
and its associated memory can be released when it is aged out
automatically or on the request of the user. Will provide the automatic release as a follow-up diff.
Test Plan: Managed* tests in db_test and XF tests for managed iterator
Reviewers: igor, yhchiang, anthony, sdong
Reviewed By: sdong
Subscribers: dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D31401
Summary:
Many link commands were identical.
Factor that out into a variable, AM_LINK, and use
it in place of all of those open-coded commands.
Test Plan: run "make check"
Reviewers: ljin, igor, sdong
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33585
Summary:
With this change, make now prints a summary line for each
compiler and linker invocation, e.g.,:
CC db/builder.o
CC db/c.o
CC db/column_family.o
To see full commands, insert "V=1" into your make command.
E.g., run "make V=1 all" if you want it to print each command
in its full glory.
$^ is GNU make's abbreviation for the prerequisites of the current target.
These AM_V_... variables expand to some very short string like "CC" or
"LD", by default, so that the output of "make" is readable. If/when you
want more details, just build with "make V=1 ...", and make will print
each full command as it is executed. If you prefer to see the noise
all the time, and only want to optionally see the abbreviated output,
set AM_DEFAULT_VERBOSITY=1 in your environment, and then build with
V=0 to see the abbreviated command indicators.
Test Plan:
invoke make a few different ways and observe:
make clean; make # abbreviated
make clean; make V=0 # also abbreviated
make clean; make V=1 # full detail
Reviewers: sdong, ljin, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33579
Summary:
This is in preparation for some factorization.
* Makefile (deletefile_test): Add $(COVERAGEFLAGS) to link command.
(options_test): Remove explicit (redundant) dependency on
options_helper.o: that is already a dependent, via $(LIBOBJECTS)
Test Plan: run make
Reviewers: sdong, ljin, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33573
Summary:
There were Makefile rules to build those two targets,
but neither rule has worked for a long time, due to missing
dependent source files. Remove those rules.
Test Plan: run "make"
Reviewers: sdong, ljin, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33567
Summary:
scan-build complains with division by zero warning in a test. Added an assertion to prevent this.
scan-build report: http://home.fburl.com/~sugak/latest6/report-c61be9.html#EndPath
Test Plan:
Make sure scan-build does not report 'Division by zero' and all tests are passing.
```lang=bash
% make analyze
% make check
```
Reviewers: igor, meyering
Reviewed By: meyering
Subscribers: sdong, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D33495
Summary:
Remove ThreadStatusMultiCompaction test as it's currently written
in a way that depends on some randomness, while the flush / compaction
status of a single thread is also covered in ThreadStatusFlush
and ThreadStatusSingleCompaction tests.
Test Plan: ./db_test
Reviewers: igor, sdong
Subscribers: dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D33537
Summary:
Add thread_status_util_debug.cc back as InstrumentedMutex related tests
are using it to produce wait that can be reflected in the counter.
Test Plan:
./perf_context_test
export ROCKSDB_TESTS=MutexWaitStats
./db_test
Reviewers: sdong, igor
Reviewed By: igor
Subscribers: dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D33525
Summary:
Before this change, running "make" with no arguments would
silently run the rules for the "uninstall" target(!). Don't do that.
* Makefile (default): New, first target; depend on "all".
(uninstall, install): Do not hide the commands we run.
Test Plan:
Run "make" and verify that the rules for "uninstall" are no longer run.
Instead, note that many files are compiled and linked. Before, none were.
Reviewers: sdong, ljin, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33531
Summary: Allow GetThreadList to reflect flush activity.
Test Plan:
Developed ThreadStatusFlush test and updated ThreadStatusMultiCompaction test.
./db_test ./thread_list_test
Reviewers: sdong, rven, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D32871
Summary:
* Makefile (dummy): Prefix this statement with "dummy := ",
so that it no longer triggers a syntax error from GNU make 3.80
and earlier. Reported by nielsl in
https://github.com/facebook/rocksdb/issues/509
Test Plan: run make
Reviewers: sdong, ljin, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33429
Summary:
In the existing implementation of `ASSERT*`, test termination happens in `~Tester`, which is called when instance of `Tester` goes out of scope. This is the cause of many scan-build bugs.
This diff changes `ASSERT*` to terminate the test immediately. Also added one suppression in `util/signal_test.cc`
scan-build bugs
before: http://home.fburl.com/~sugak/latest/index.html
after: http://home.fburl.com/~sugak/latest2/index.html
Test Plan:
Modify some test to fail an assertion and make sure that `ASSERT*` terminated the test.
Run `make analyze` and make sure no 'Called C++ object pointer is null' and 'Dereference of null pointer' bugs reported.
Run tests and make sure no failing tests:
```lang=bash
% make check
% USE_CLANG=1 make check
```
Reviewers: meyering, lgalanis, sdong, rven, igor
Reviewed By: igor
Subscribers: dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D33381
Summary:
The code being removed would invoke sed differently to work
around a portability difference in how sed -i works (different
on MacOS). Yet performing a host-type-based ifdef fails when
the tools installed do not match. That sed use was solely to
post-process the .d file. Instead, generate the desired output
directly, by using the compiler's -MT<FILE> option.
* Makefile (%.d: %.cc): With the prior use of Makefile-ifdef'd
sed, when building on MacOS with gnu sed, every run of this rule
would fail with a sed usage error. Also list each .d file as a
dependent.
Test Plan:
Ensure that a selected .d file is the same as before both with
g++ and with clang++. However, note that the new .d files each
contain a new reference to the .d file itself.
Reviewers: sdong, ljin, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33369
Summary:
It would be good to assing background job their IDs. Two benefits:
1) makes LOGs more readable
2) I might use it in my EventLogger, which will try to make our LOG easier to read/query/visualize
Test Plan: ran rocksdb, read the LOG
Reviewers: sdong, rven, yhchiang
Reviewed By: yhchiang
Subscribers: dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D31617
Summary: rockuse more memory that asked to. Monitor and report.
Test Plan: run the pro with conditions to simulate the overusage. It should report that the process is using more memory than needed.
Reviewers: yhchiang, rven, sdong, igor
Reviewed By: igor
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33249
Summary: DBTest.DestroyDBMetaDatabase occasionally fails on my dev host, for file not existing. Always create directories to avoid that.
Test Plan: Run the test
Reviewers: rven, yhchiang, igor
Reviewed By: igor
Subscribers: dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D33321
Summary: Remember whole key or prefix filtering on/off in SST files. If user opens the DB with a different setting that cannot be satisfied while reading the SST file, ignore the bloom filter.
Test Plan: Add a unit test for it
Reviewers: yhchiang, igor, rven
Reviewed By: rven
Subscribers: leveldb, dhruba
Differential Revision: https://reviews.facebook.net/D32889
Summary:
Bytes are currently misinterpreted by the Java if the
byte array contains zero bytes within its content. For Strings
thats usually not useful. As the Java API allows every kind
of byte array values it might be the case that zero padding might
happen.
Test Plan:
make rocksdbjava
make jtest
Reviewers: adamretter, yhchiang, ankgup87
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D33165
Summary: Add counters in perf context to allow users to figure out how time spent on waiting for DB mutex
Test Plan: Add a test and run it.
Reviewers: yhchiang, rven, igor
Reviewed By: igor
Subscribers: leveldb, dhruba
Differential Revision: https://reviews.facebook.net/D33177
Summary: For description of the bug, see comment in db_test. The fix is pretty straight forward.
Test Plan: added unit test. eventually we need better testing of FOF/POF process.
Reviewers: yhchiang, rven, sdong
Reviewed By: sdong
Subscribers: dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D33081
Summary:
- In statistics.h , added tickers.
- In version_set.cc,
-- Added a getter method for hit_file_level_ in the class FilePicker
-- Added a line in the Get() method in case of a found, increment the corresponding counters based on the level of the file respectively.
Corresponding task: https://our.intern.facebook.com/intern/tasks/?s=506100481&t=5952818
Personal fork: 0c3f2e3600
Test Plan:
In terminal,
```
make -j32 db_test
ROCKSDB_TESTS=L0L1L2AndUpHitCounter ./db_test
```
Or to use debugger,
```
make -j32 db_test
export ROCKSDB_TESTS=L0L1L2AndUpHitCounter
gdb db_test
```
Reviewers: rven, sdong
Reviewed By: sdong
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D32205
Summary: Having a pointer for DB will be helpful to debug when GDB or working on a dump. If the client process doesn't have any thread actively working on RocksDB, it can be hard to find out.
Test Plan: make all check
Reviewers: rven, yhchiang, igor
Reviewed By: igor
Subscribers: yoshinorim, dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D33159
Summary:
This change adds LogHeader provision to the logger. For the rolling logger
implementation, the headers are copied over to the new log file every time
there is a log roll over.
Test Plan: Added a unit test to test the rolling log case.
Reviewers: sdong
Subscribers: dhruba
Differential Revision: https://reviews.facebook.net/D32817
Summary:
This diff basically reverts D30249 and also adds a unit test that was failing before this patch.
I have no idea how I didn't catch this terrible bug when writing a diff, sorry about that :(
I think we should redesign our system of keeping track of and deleting files. This is already a second bug in this critical piece of code. I'll think of few ideas.
BTW this diff is also a regression when running lots of column families. I plan to revisit this separately.
Test Plan: added a unit test
Reviewers: yhchiang, rven, sdong
Reviewed By: sdong
Subscribers: dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D33045
Summary:
When DestroyDB() finds a wal file in the DB directory, it assumes it is actually in WAL directory. This can lead to confusion, since it reports IO error when it tries to delete wal file from DB directory. For example: https://ci-builds.fb.com/job/rocksdb_clang_build/296/console
This change will fix our unit tests.
Test Plan: unit tests work
Reviewers: yhchiang, rven, sdong
Reviewed By: sdong
Subscribers: dhruba, leveldb
Differential Revision: https://reviews.facebook.net/D32907
