This CL makes it easier to reason about thread safety by:
1) Adding Clang thread safety annotations according to comments.
2) Expanding a couple of variable names, without adding extra lines of code.
3) Adding const in a couple of places.
4) Replacing an always-non-null const pointer with a reference.
5) Fixing style warnings in the modified files.
This CL does not annotate the DBImpl members that claim to be protected
by the instance mutex, but are accessed without the mutex being held.
Those members (and their unprotected accesses) will be addressed in
future CLs.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=189354657
This CL removes unused headers included by util/testharness.h, adds
precise includes where the build breaks, and fixes style errors in the
edited files.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=189331061
External linkage is the default for function declarations in C++.
This also fixes ClangTidy errors generated by removing the "extern"
keyword as described above.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=188730416
This is a stopgap for removing warnings on Mac builds, so -Werror can be
turned on. C++11 will be required in the nearby future, which guarantees
<atomic> support. Once that happens, the simplified version of this will
match https://github.com/google/leveldb/pull/503
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=188553251
This aligns the Travis CI configuration with google/crc32c and
google/snappy, to simplify maintenance.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=188547648
This removes the use of the non-portable headers <sys/types.h> and <unistd.h> in c_test.c.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=188503102
When the max file size option was added in CL 134391640 the C API
was not modified to support this.
This change was contributed by GitHub user @olt and fixes issue #439.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=173466388
The C++ style guide URL was wrong.
This fixes issue #394. Reported by GitHub user @Loki-Astari.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=173188573
Deleted two unused assignments:
1. offset_in_block in Reader::SkipToInitialBlock().
2. in_fragmented_record in Reader::ReadRecord().
Reasons for the change:
1. offset_in_block is not read again after the if condition.
2. The kFullRecordType switch branch returns, so
in_fragmented_record isn't read again.
3. The kFirstType switch branch sets in_fragmented_record to
true after the if, so the write in the if is ignored.
Change contributed by @C0deAi on GitHub.
This fixes https://github.com/google/leveldb/issues/517
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=172763897
This follows the general naming convention for preprocessor macros used
to detect feature (library / header file / symbol) presence.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=171184641
CL 170738066 removed all instances of fread_unlocked, fwrite_unlocked
and fflush_unlocked calls from the codebase, so the feature detection
can be removed as well.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=171154269
gcc defaults to exporting all symbols, but other linkers do not. Adding
the LEVELDB_EXPORT macro allows a project to set LEVELDB_SHARED_LIBRARY
when building/linking with leveldb as a shared library.
This is to allow leveldb to be created as a shared library on all
platforms support by Chrome and enables a fix for
https://bugs.chromium.org/p/chromium/issues/detail?id=764810.
This also has the benefit of reducing the shared library size from
418863 to 380367 bytes (64-bit Linux).
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=171037148
CL 170738066 introduced std::min and std::max to env_test.cc. These
require the <algorithm> header.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=171024062
Deleting a PosixWritableFile without calling Close() leaks the file
descriptor. While the API description in include/leveldb/env.h does not
specify whether the caller is responsible for Close()ing the file before
deleting it, all other Env file implementations do release underlying
resources when destroyed, even if Close() is not called.
The leak shows up when running db_tests on Mac Travis, or on a vanilla
MacOS install.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170906843
This also removes std::unique_ptr introduced in CL 170738066, because
it's C++11-only, and the open source version still supports older
versions at the moment.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170876919
If the file already existed, we should have truncated it. This was not
detected by leveldb tests since leveldb code avoids reusing same files,
but there was code elsewhere that was directly using leveldb files and
relying on this behavior.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170769101
If leveldb::Options::block_cache is set to a cache of zero capacity
then it is possible for LRUHandle::next to be used without having been
set.
Conditional jump or move depends on uninitialised value(s):
leveldb::(anonymous namespace)::LRUHandle::key() const (cache.cc:58)
leveldb::(anonymous namespace)::LRUCache::Unref(leveldb::(anonymous namespace)::LRUHandle*) (cache.cc:234)
leveldb::(anonymous namespace)::LRUCache::Release(leveldb::Cache::Handle*) (cache.cc:266)
leveldb::(anonymous namespace)::ShardedLRUCache::Release(leveldb::Cache::Handle*) (cache.cc:375)
leveldb::CacheTest::Insert(int, int, int) (cache_test.cc:59)
This bug forced a commit reversion in Chromium. For more information see
https://bugs.chromium.org/p/chromium/issues/detail?id=761398#c4
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170749054
env_posix.cc and concurrent application calls to fflush(NULL).
The fix is to avoid using stdio in env_posix.cc but add our own
buffering where we need it.
Added a test to reproduce the bug.
Added a test for Env reads/writes.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=170738066
12 lines above, there is an "if (!s.ok()) { return s; }" block of code.
"s" is never modified between that block and the "if" removed by this
CL, so "s.ok()" must be true.
The code most likely intended to say "if (!builder->ok())", because the
builder->Add() call above can modify the TableBuilder's status, as a
side-effect. However, this approach would have required setting "s =
builder.status()" in the "else" branch, near the "builder.Abandon()"
call. So, removing the "if" outright is simpler than following that line
of thought.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=167326229
An Android test was occasionally crashing with a SEGV in ConsumeDecimalNumber
Switching a local variable from an int to uint64_t eliminated these crashes.
Speculating this is either a compiler, runtime library, or emulator issue.
Switching this type to uint64_t also eliminates a compiler warning
about comparing an int with a uint64_t.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=166399695
When faced with a pointer that is misaligned by K bytes (pointer % 8 ==
K), the code previously moved forward by K bytes. In order to end up
with an aligned pointer, the code must move by 8 - K bytes.
This lands https://github.com/google/leveldb/pull/488
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=166295921
The dead code has been in the codebase since the initial commit and is
generating a compiler warning when used in Xcode.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=164174594
Use __APPLE__ instead of OS_MACOS when testing for the Apple platform and
remove the latter symbol from the BUILD file. This fixes incompatibility issues
when using the library on an Apple device.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=162958094
BTRFS reorders rename and write operations, so it is possible that a filesystem crash and recovery results in a situation where the file pointed to by CURRENT does not exist. DB::Open currently reports an I/O error in this case. Reporting database corruption is a better hint to the caller, which can attempt to recover the database or erase it and start over.
This issue is not merely theoretical. It was reported as having showed up in the wild at https://github.com/google/leveldb/issues/195 and at https://crbug.com/738961. Also, asides from the BTRFS case described above, incorrect data in CURRENT seems like a possible corruption case that should be handled gracefully.
The Env API changes here can be considered backwards compatible, because an implementation that returns Status::IOError instead of Status::NotFound will still get the same functionality as before.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=161432630
env_test.cc defines EnvPosixTest which tests the Env implementation returned by Env::Default(). The naming is a bit unfortunate, as the tests in env_test.cc are written against the Env contract, and therefore are applicable to any Env implementation. An instance of the confusion caused by the naming is [] which added a dependency from env_test.cc to EnvPosixTestHelper, which is closely coupled to EnvPosix.
This change disentangles EnvPosix-specific test code into a env_posix_test.cc file. The code there uses EnvPosixTestHelper and specifically targets the EnvPosix implementation. env_test.cc now implements EnvTest, and contains tests that are also applicable to other ports, which may define their own Env implementation.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148914642
LE_LOAD64 is only used when _mm_crc32_u64 is available, on 64-bit x86 processors.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148906169
Prior index.html was using rawgit.com which doesn't process
Markdown and therefore only serves the markdown source.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148902180
Markdown is more readable in a text editor and when hosted
on GitHub is more readable than HTML.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148830423
This change authored by vadimskipin and submitted via:
https://github.com/google/leveldb/pull/309
Changes made to support iOS builds and other architectures
without support for SSE 4.2.
db_bench reports original crc32 speed at:
crc32c : 3.610 micros/op; 1082.0 MB/s (4K per op)
with this change performance has increased to:
crc32c : 0.843 micros/op; 4633.6 MB/s (4K per op)
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148694935
Background compaction can create an unbounded number of
leveldb::RandomAccessFile instances. On 64-bit systems mmap is used and
file descriptors are only used beyond a certain number of mmap's.
32-bit systems to not use mmap at all. leveldb::RandomAccessFile does not
observe Options.max_open_files so compaction could exhaust the file
descriptor limit.
This change uses getrlimit to determine the maximum number of open
files and limits RandomAccessFile to approximately 20% of that value.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=143505556
Background:
LevelDB uses a cache (util/cache.h, util/cache.cc) of (key,value)
pairs for two purposes:
- a cache of (table, file handle) pairs
- a cache of blocks
The cache places the (key,value) pairs in a reference-counted
wrapper. When it returns a value, it returns a reference to this
wrapper. When the client has finished using the reference and
its enclosed (key,value), it calls Release() to decrement the
reference count.
Each (key,value) pair has an associated resource usage. The
cache maintains the sum of the usages of the elements it holds,
and removes values as needed to keep the sum below a capacity
threshold. It maintains an LRU list so that it will remove the
least-recently used elements first.
The max_open_files option to LevelDB sets the size of the cache
of (table, file handle) pairs. The option is not used in any
other way.
The observed behaviour:
If LevelDB at any time used more file handles concurrently than
the cache size set via max_open_files, it attempted to reduce the
number by evicting entries from the table cache. This could
happen most easily during compaction, and if max_open_files was
low. Because the handles were in use, their reference count did
not drop to zero, and so the usage sum in the cache was not
modified by the evictions. Subsequent Insert() calls returned
valid handles, but their entries were immediately evicted from
the cache, which though empty still acted as though full. As a
result, there was effectively no caching, and the number of open
file handles rose []ly until it hit system-imposed limits and
the process died.
If one set max_open_files lower, the cache was more likely to
exhibit this beahviour, and cause the process to run out of file
descriptors. That is, max_open_files acted in almost exactly the
opposite manner from what was intended.
The problems:
1. The cache kept all elements on its LRU list eligible for capacity
eviction---even those with outstanding references from clients. This was
ineffective in reducing resource consumption because there was an
outstanding reference, guaranteeing that the items remained. A secondary
issue was that there is no guarantee that these in-use items will be the
last things reached in the LRU chain, which actually recorded
"least-recently requested" rather than "least-recently used".
2. The sum of usages was decremented not when a (key,value) was evicted from
the cache, but when its reference count went to zero. Thus, when things
were removed from the cache, either by garbage collection or via Erase(),
the usage sum was not necessarily decreased. This allowed the cache to act
as though full when it was in fact not, reducing caching effectiveness, and
leading to more resources being consumed---the opposite of what the
evictions were intended to achieve.
3. (minor) The cache's clients insert items into it by first looking up the
key, and inserting only if no value is found. Although the cache has an
internal lock, the clients use no locking to ensure atomicity of the
Lookup/Insert pair. (see table/table.cc: block_cache->Insert() and
db/table_cache.cc: cache_->Insert()). Thus, if two threads Insert() at
about the same time, they can both Lookup(), find nothing, and both
Insert(). The second Insert() would evict the first value, leaving each
thread with a handle on its own version of the data, and with the second
version in the cache. It would be better if both threads ended up with a
handle on the same (key,value) pair, which implies it must be the first item
inserted. This suggests that Insert() should not replace an existing value.
This can be made safe with current usage inside LeveDB itself, but this is
not easy to change first because Cache is a public interface, so to change
the semantics of an existing call might break things, second because Cache
is an abstract virtual class, so adding a new abstract virtual method may
break other implementations, and third, the new method "insert without
replacing" cannot be implemented in terms of the existing methods, so cannot
be implemented with a non-abstract default. But fortunately, the effects
of this issue are minor, so this issue is not fixed by this change.
The changes:
The assumption in the fixes is that it is always better to cache
entries unless removal from the cache would lead to deallocation.
Cache entries now have an "in_cache" boolean indicating whether
the cache has a reference on the entry. The only ways that this can
become false without the entry being passed to its "deleter" are via
Erase(), via Insert() when an element with a duplicate key is inserted,
or on destruction of the cache.
The cache now keeps two linked lists instead of one. All items
in the cache are in one list or the other, and never both. Items
still referenced by clients but erased from the cache are in
neither list. The lists are:
- in-use: contains the items currently referenced by clients, in no particular
order. (This list is used for invariant checking. If we removed the check,
elements that would otherwise be on this list could be left as disconnected
singleton lists.)
- LRU: contains the items not currently referenced by clients, in LRU order
A new internal Ref() method increments the reference count. If
incrementing from 1 to 2 for an item in the cache, it is moved
from the LRU list to the in-use list.
The Unref() call now moves things from the in-use list to the LRU
list if the reference count falls to 1, and the item is in the
cache. It no longer adjusts the usage sum. The usage sum now
reflects only what is in the cache, rather than including
still-referenced items that have been evicted.
The LRU_Append() now takes a "list" parameter so that it can be
used to append either to the LRU list or the in-use list.
Lookup() is modified to use the new Ref() call, rather than
adjusting the reference count and LRU chain directly.
Insert() eviction code is also modified to adjust the usage sum and the
in_cache boolean of the evicted elements. Some LevelDB tests assume that there
will be no caching whatsoever if the cache size is set to zero, so this is
handled as a special case.
A new private method FinishErase() is factored out
with the common code from where items are removed from the cache.
Erase() is modified to adjust the usage sum and the in_cache
boolean of the erased elements, and to use FinishErase().
Prune() is modified to use FinishErase() also, and to make use of the fact that
the lru_ list now contains only items with reference count 1.
- EvictionPolicy is modified to test that an entry with an
outstanding handle is not evicted. This test fails with the old cache.cc.
- A new test case UseExceedsCacheSize verifies that even when the
cache is overfull of entries with outstanding handles, none are
evicted. This test fails with the old cache.cc, and is the key
issue that causes file descriptors to run out when the cache
size is set too small.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=123247237
Detailed description:
Suppose an input split is generated between two leveldb record blocks and the preceding block ends with null padding.
A reader that previously read at least 1 record within the first block (before encountering the padding) upon trying to read the next record, will successfully and correctly read the next logical record from the subsequent block, but will return a last record offset pointing to the padding in the first block.
When this happened in a [], it resulted in duplicate records being handled at what appeared to be different offsets that were separated by only a few bytes.
This behavior is only observed when at least 1 record was read from the first block before encountering the padding. If the initial offset for a reader was within the padding, the correct record offset would be reported, namely the offset within the second block.
The tests failed to catch this scenario/bug, because each read test only read a single record with an initial offset. This CL adds an explicit test case for this scenario, and modifies the test structure to read all remaining records in the test case after an initial offset is specified. Thus an initial offset that jumps to record #3, with 5 total records in the test file, will result in reading 2 records, and validating the offset of each of them in order to pass successfully.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=115338487
costan
cmumford
sanjay
davidair
scrubbed
corrado
m3b
John Abd-El-Malek
Nicholas Westlake
mjwiacek