Permalink
Browse files

fixes to item_walk, a unit test for item_walk

Summary: item_walk had some bugs for extracting the middle part of an item.  we never do that (we get the entire item for arithmetic/send/receive, and we get the end for memset/stamping), so this probably shouldn't actually make a difference.  still, it is good to fix.
         
         also added some unit tests for item_walk (which is how the bugs were found).
         
         This includes http://www.dev.facebook.com/intern/diffcamp/?tab=review&revisionID=13628

Reviewed By: marc

Test Plan: all flat allocator tests pass

Revert: OK


git-svn-id: http://svn.facebook.com/svnroot/projects/memcached/trunk@103964 2c7ba8d8-a2f7-0310-a573-de162e16dcc7
  • Loading branch information...
1 parent 55ea2cc commit 434ed3954b7c391b88d530fb112f844161a0d574 ttung committed Jun 12, 2008
Showing with 66 additions and 30 deletions.
  1. +0 −1 binary_protocol.h
  2. +8 −4 flat_storage.c
  3. +17 −8 flat_storage.h
  4. +12 −1 memcached.c
  5. +1 −1 slabs_items.c
  6. +15 −5 stats.c
  7. +3 −0 t/64bit.t
  8. +2 −2 t/lib/MemcachedTest.pm
  9. +6 −6 t/stats-detail.t
  10. +2 −2 t/stats.t
View
@@ -70,7 +70,6 @@ typedef enum bp_cmd {
BP_STATS_CMD = (BP_S_S | FIELD(0x0, 0)),
} bp_cmd_t;
-// these commands
#define BINARY_PROTOCOL_REQUEST_HEADER \
uint8_t magic; \
View
@@ -707,7 +707,7 @@ static coalesce_progress_t coalesce_free_small_chunks(rel_time_t large_lru_item_
} else {
/* body block. this is more straightforward */
small_chunk_t* prev_chunk = &(get_chunk_address(replacement->sc_body.prev_chunk))->sc;
- small_chunk_t* next_chunk = &(get_chunk_address(replacement->sc_body.next_chunk))->sc;;
+ small_chunk_t* next_chunk = &(get_chunk_address(replacement->sc_body.next_chunk))->sc;
/* update the previous block's next pointer */
if (prev_chunk->flags & SMALL_CHUNK_TITLE) {
@@ -979,6 +979,10 @@ void do_try_item_stamp(item* it, rel_time_t now, const struct in_addr addr) {
* initialized. if there is insufficient memory, NULL is returned. */
item* do_item_alloc(char *key, const size_t nkey, const int flags, const rel_time_t exptime,
const size_t nbytes, const struct in_addr addr) {
+ if (item_size_ok(nkey, flags, nbytes) == false) {
+ return NULL;
+ }
+
if (is_large_chunk(nkey, nbytes)) {
/* allocate a large chunk */
@@ -1272,7 +1276,7 @@ static void item_free(item *it) {
* the maximum for a cache entry.)
*/
bool item_size_ok(const size_t nkey, const int flags, const int nbytes) {
- return (nkey < KEY_MAX_LENGTH) && (nbytes < MAX_ITEM_SIZE);
+ return (nkey <= KEY_MAX_LENGTH) && (nbytes <= MAX_ITEM_SIZE);
}
@@ -1417,10 +1421,10 @@ void do_item_unlink(item* it, long flags) {
} else if (flags & UNLINK_IS_EXPIRED) {
stats_expire(ITEM_nkey(it) + ITEM_nbytes(it));
}
- STATS_UNLOCK();
if (settings.detail_enabled) {
stats_prefix_record_removal(ITEM_key(it), ITEM_nkey(it) + ITEM_nbytes(it), it->empty_header.time, flags);
}
+ STATS_UNLOCK();
assoc_delete(ITEM_key(it), ITEM_nkey(it), ITEM_PTR(it));
it->empty_header.h_next = NULL_ITEM_PTR;
item_unlink_q(it);
@@ -1722,7 +1726,7 @@ char* do_flat_allocator_stats(size_t* result_size) {
"STAT unused_memory %lu\n"
"STAT large_free_list_sz %lu\n"
"STAT small_free_list_sz %lu\n"
- "STAT oldest_item_lifetime %lus\n",
+ "STAT oldest_item_lifetime %us\n",
fsi.stats.break_events,
fsi.stats.unbreak_events,
fsi.stats.migrates,
View
@@ -721,8 +721,8 @@ STATIC_DECL(item* get_lru_item(chunk_type_t chunk_type, small_title_chunk_t* sta
end_offset = LARGE_TITLE_CHUNK_DATA_SZ - \
((_it)->empty_header.nkey) - 1; \
} else { \
- end_offset = __fss_MIN(left, \
- LARGE_TITLE_CHUNK_DATA_SZ - ((_it)->empty_header.nkey)) - 1; \
+ end_offset = __fss_MIN((_offset) + (_nbytes), \
+ start_offset + LARGE_TITLE_CHUNK_DATA_SZ - ((_it)->empty_header.nkey)) - 1; \
} \
to_scan = end_offset - start_offset + 1; \
} else { \
@@ -739,7 +739,8 @@ STATIC_DECL(item* get_lru_item(chunk_type_t chunk_type, small_title_chunk_t* sta
if (next == NULL && (_beyond_item_boundary)) { \
end_offset = LARGE_BODY_CHUNK_DATA_SZ - 1; \
} else { \
- end_offset = __fss_MIN(left, LARGE_BODY_CHUNK_DATA_SZ) - 1; \
+ end_offset = __fss_MIN((_offset) + (_nbytes), \
+ LARGE_BODY_CHUNK_DATA_SZ) - 1; \
} \
to_scan = end_offset - start_offset + 1; \
} \
@@ -774,7 +775,8 @@ STATIC_DECL(item* get_lru_item(chunk_type_t chunk_type, small_title_chunk_t* sta
(_beyond_item_boundary)) { \
end_offset = start_offset + LARGE_BODY_CHUNK_DATA_SZ - 1; \
} else { \
- end_offset = start_offset + __fss_MIN(left, LARGE_BODY_CHUNK_DATA_SZ) - 1; \
+ end_offset = __fss_MIN((_offset) + (_nbytes), \
+ start_offset + LARGE_BODY_CHUNK_DATA_SZ) - 1; \
} \
to_scan = end_offset - start_offset + 1; \
} while (start_offset <= (_it)->empty_header.nbytes); \
@@ -792,8 +794,8 @@ STATIC_DECL(item* get_lru_item(chunk_type_t chunk_type, small_title_chunk_t* sta
end_offset = SMALL_TITLE_CHUNK_DATA_SZ - \
((_it)->empty_header.nkey) - 1; \
} else { \
- end_offset = __fss_MIN(left, \
- SMALL_TITLE_CHUNK_DATA_SZ - ((_it)->empty_header.nkey)) - 1; \
+ end_offset = __fss_MIN((_offset) + (_nbytes), \
+ start_offset + SMALL_TITLE_CHUNK_DATA_SZ - ((_it)->empty_header.nkey)) - 1; \
} \
to_scan = end_offset - start_offset + 1; \
} else { \
@@ -810,11 +812,14 @@ STATIC_DECL(item* get_lru_item(chunk_type_t chunk_type, small_title_chunk_t* sta
if (next == NULL && (_beyond_item_boundary)) { \
end_offset = SMALL_BODY_CHUNK_DATA_SZ - 1; \
} else { \
- end_offset = __fss_MIN(left, SMALL_BODY_CHUNK_DATA_SZ) - 1; \
+ end_offset = __fss_MIN((_offset) + (_nbytes), \
+ start_offset + SMALL_BODY_CHUNK_DATA_SZ) - 1; \
} \
to_scan = end_offset - start_offset + 1; \
} \
\
+ /* printf(" leaving head region with end_offset = %ld\n", end_offset); */ \
+ \
/* advance over pages writing while doing the requested action. */ \
do { \
/* offset must be less than end offset. */ \
@@ -831,6 +836,8 @@ STATIC_DECL(item* get_lru_item(chunk_type_t chunk_type, small_title_chunk_t* sta
left -= work_len; \
} \
\
+ /* printf(" left = %lu\n", left); */ \
+ \
if (left == 0) { \
break; \
} \
@@ -845,8 +852,10 @@ STATIC_DECL(item* get_lru_item(chunk_type_t chunk_type, small_title_chunk_t* sta
(_beyond_item_boundary)) { \
end_offset = start_offset + SMALL_BODY_CHUNK_DATA_SZ - 1; \
} else { \
- end_offset = start_offset + __fss_MIN(left, SMALL_BODY_CHUNK_DATA_SZ) - 1; \
+ end_offset = __fss_MIN((_offset) + (_nbytes), \
+ start_offset + SMALL_BODY_CHUNK_DATA_SZ) - 1; \
} \
+ /* printf(" cycling start_offset = %ld, end_offset = %ld\n", start_offset, end_offset); */ \
to_scan = end_offset - start_offset + 1; \
} while (start_offset <= (_it)->empty_header.nbytes); \
} \
View
@@ -394,7 +394,12 @@ conn *conn_new(const int sfd, const int init_state, const int event_flags,
return NULL;
}
memcpy(&c->request_addr, addr, addrlen);
- c->request_addr_size = addrlen;
+ if (settings.socketpath) {
+ c->request_addr_size = 0; /* for unix-domain sockets, don't store
+ * a request addr. */
+ } else {
+ c->request_addr_size = addrlen;
+ }
c->rbuf = c->wbuf = 0;
c->ilist = 0;
@@ -1080,6 +1085,12 @@ static void process_stat(conn* c, token_t *tokens, const size_t ntokens) {
offset = append_to_buffer(temp, bufsize, offset, sizeof(terminator), "STAT time %ld\r\n", now + stats.started);
offset = append_to_buffer(temp, bufsize, offset, sizeof(terminator), "STAT version " VERSION "\r\n");
offset = append_to_buffer(temp, bufsize, offset, sizeof(terminator), "STAT pointer_size %lu\r\n", 8 * sizeof(void *));
+#if defined(USE_SLAB_ALLOCATOR)
+ offset = append_to_buffer(temp, bufsize, offset, sizeof(terminator), "STAT allocator slab\r\n");
+#endif /* #if defined(USE_SLAB_ALLOCATOR) */
+#if defined(USE_FLAT_ALLOCATOR)
+ offset = append_to_buffer(temp, bufsize, offset, sizeof(terminator), "STAT allocator flat\r\n");
+#endif /* #if defined(USE_FLAT_ALLOCATOR) */
#ifndef WIN32
offset = append_to_buffer(temp, bufsize, offset, sizeof(terminator), "STAT rusage_user %ld.%06d\r\n", usage.ru_utime.tv_sec, (int) usage.ru_utime.tv_usec);
offset = append_to_buffer(temp, bufsize, offset, sizeof(terminator), "STAT rusage_system %ld.%06d\r\n", usage.ru_stime.tv_sec, (int) usage.ru_stime.tv_usec);
View
@@ -310,10 +310,10 @@ void do_item_unlink_impl(item *it, long flags, bool to_freelist) {
} else if (flags & UNLINK_IS_EXPIRED) {
stats_expire(it->nkey + it->nbytes);
}
- STATS_UNLOCK();
if (settings.detail_enabled) {
stats_prefix_record_removal(ITEM_key(it), it->nkey + it->nbytes, it->time, flags);
}
+ STATS_UNLOCK();
assoc_delete(ITEM_key(it), it->nkey, it);
item_unlink_q(it);
if (it->refcount == 0) {
View
@@ -340,11 +340,21 @@ char *stats_prefix_dump(int *length) {
lifetime = wildcard.total_lifetime / wildcard.num_items;
}
- offset = append_to_buffer(buf, size, offset, sizeof(terminator),
- format,
- "*wildcard*", wildcard.num_items, wildcard.num_gets, wildcard.num_hits,
- wildcard.num_sets, wildcard.num_deletes, wildcard.num_evicts,
- wildcard.num_bytes, lifetime, wildcard.total_byte_seconds);
+ if (wildcard.num_items != 0 ||
+ wildcard.num_gets != 0 ||
+ wildcard.num_hits != 0 ||
+ wildcard.num_sets != 0 ||
+ wildcard.num_deletes != 0 ||
+ wildcard.num_evicts != 0 ||
+ wildcard.num_bytes != 0 ||
+ lifetime != 0 ||
+ wildcard.total_byte_seconds != 0) {
+ offset = append_to_buffer(buf, size, offset, sizeof(terminator),
+ format,
+ "*wildcard*", wildcard.num_items, wildcard.num_gets, wildcard.num_hits,
+ wildcard.num_sets, wildcard.num_deletes, wildcard.num_evicts,
+ wildcard.num_bytes, lifetime, wildcard.total_byte_seconds);
+ }
STATS_UNLOCK();
offset = append_to_buffer(buf, size, offset, 0, terminator);
View
@@ -19,6 +19,9 @@ $stats = mem_stats($sock);
if ($stats->{'pointer_size'} eq "32") {
plan skip_all => 'Skipping 64-bit tests on 32-bit build';
exit 0;
+} elsif ($stats->{'allocator'} eq "flat") {
+ plan skip_all => 'Skipping 64-bit tests on flat allocator build';
+ exit 0;
} else {
plan tests => 6;
}
@@ -21,7 +21,7 @@ sub mem_stats {
my $stats = {};
while (<$sock>) {
last if /^(\.|END)/;
- /^STAT (\S+) (\d+)/;
+ /^STAT (\S+) (\S+)/;
#print " slabs: $_";
$stats->{$1} = $2;
}
@@ -104,7 +104,7 @@ sub new_memcached {
if ($args =~ /-s (\S+)/) {
sleep 1;
my $filename = $1;
- my $conn = IO::Socket::UNIX->new(Peer => $filename) ||
+ my $conn = IO::Socket::UNIX->new(Peer => $filename) ||
croak("Failed to connect to unix domain socket: $! '$filename'");
return Memcached::Handle->new(pid => $childpid,
View
@@ -20,25 +20,25 @@ print $sock "set foo:123 0 0 6\r\nfooval\r\n";
is(scalar <$sock>, "STORED\r\n", "stored foo");
print $sock "stats detail dump\r\n";
-is(scalar <$sock>, "PREFIX foo get 0 hit 0 set 1 del 0\r\n", "details after set");
+is(scalar <$sock>, "PREFIX foo item 1 get 0 hit 0 set 1 del 0 evict 0 bytes 13 avrg lifetime 0 byte-seconds 0\r\n", "details after set");
is(scalar <$sock>, "END\r\n", "end of details");
mem_get_is($sock, "foo:123", "fooval");
print $sock "stats detail dump\r\n";
-is(scalar <$sock>, "PREFIX foo get 1 hit 1 set 1 del 0\r\n", "details after get with hit");
+is(scalar <$sock>, "PREFIX foo item 1 get 1 hit 1 set 1 del 0 evict 0 bytes 13 avrg lifetime 0 byte-seconds 0\r\n", "details after get with hit");
is(scalar <$sock>, "END\r\n", "end of details");
mem_get_is($sock, "foo:124", undef);
print $sock "stats detail dump\r\n";
-is(scalar <$sock>, "PREFIX foo get 2 hit 1 set 1 del 0\r\n", "details after get without hit");
+is(scalar <$sock>, "PREFIX foo item 1 get 2 hit 1 set 1 del 0 evict 0 bytes 13 avrg lifetime 0 byte-seconds 0\r\n", "details after get without hit");
is(scalar <$sock>, "END\r\n", "end of details");
print $sock "delete foo:125 0\r\n";
is(scalar <$sock>, "NOT_FOUND\r\n", "sent delete command");
print $sock "stats detail dump\r\n";
-is(scalar <$sock>, "PREFIX foo get 2 hit 1 set 1 del 1\r\n", "details after delete");
+is(scalar <$sock>, "PREFIX foo item 1 get 2 hit 1 set 1 del 1 evict 0 bytes 13 avrg lifetime 0 byte-seconds 0\r\n", "details after delete");
is(scalar <$sock>, "END\r\n", "end of details");
print $sock "stats reset\r\n";
@@ -49,7 +49,7 @@ is(scalar <$sock>, "END\r\n", "empty stats after clear");
mem_get_is($sock, "foo:123", "fooval");
print $sock "stats detail dump\r\n";
-is(scalar <$sock>, "PREFIX foo get 1 hit 1 set 0 del 0\r\n", "details after clear and get");
+is(scalar <$sock>, "PREFIX foo item 0 get 1 hit 1 set 0 del 0 evict 0 bytes 0 avrg lifetime 0 byte-seconds 0\r\n", "details after clear and get");
is(scalar <$sock>, "END\r\n", "end of details");
print $sock "stats detail off\r\n";
@@ -59,5 +59,5 @@ mem_get_is($sock, "foo:124", undef);
mem_get_is($sock, "foo:123", "fooval");
print $sock "stats detail dump\r\n";
-is(scalar <$sock>, "PREFIX foo get 1 hit 1 set 0 del 0\r\n", "details after stats turned off");
+is(scalar <$sock>, "PREFIX foo item 0 get 1 hit 1 set 0 del 0 evict 0 bytes 0 avrg lifetime 0 byte-seconds 0\r\n", "details after stats turned off");
is(scalar <$sock>, "END\r\n", "end of details");
View
@@ -37,10 +37,10 @@ my $sock = $server->sock;
my $stats = mem_stats($sock);
# Test number of keys
-is(scalar(keys(%$stats)), 24, "24 stats values");
+is(scalar(keys(%$stats)), 31, "31 stats values");
# Test initial state
-foreach my $key (qw(curr_items total_items bytes cmd_get cmd_set get_hits evictions get_misses bytes_written)) {
+foreach my $key (qw(curr_items total_items item_total_size cmd_get cmd_set get_hits evictions get_misses bytes_written)) {
is($stats->{$key}, 0, "initial $key is zero");
}

0 comments on commit 434ed39

Please sign in to comment.