Re: [Tarantool-patches] [PATCH v2 07/10] vinyl: align statements and bps tree extents

["Timur Safin" <tsafin@tarantool.org>]

Agreed! LGTM!

: -----Original Message-----
: From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
: Sent: Thursday, May 28, 2020 2:32 AM
: To: tarantool-patches@dev.tarantool.org; alyapunov@tarantool.org;
: korablev@tarantool.org; tsafin@tarantool.org
: Subject: [PATCH v2 07/10] vinyl: align statements and bps tree extents
: 
: Vinyl tuples (vy_stmt) in 0 level of LSM tree are stored in
: lsregion. They were allocated using lsregion_alloc(), which does
: not align its results, and is good only for byte arrays.
: 
: As a result, vy_stmt object addresses in 0 LSM level were not
: aligned. Unaligned memory access is slower, and may even crash on
: some platforms.
: 
: Besides, even aligned allocations couldn't help upserts in 0 level
: of the LSM tree, because upsert vy_stmt objects had 1 byte prefix
: to count merged upserts stored in this statement. This 1 byte
: prefix ruined all the alignment. Now the upsert counter is also
: aligned, the same as vy_stmt. Note, it does not consume
: significantly more memory, since it used only for vinyl and only
: for upserts, stored in 0 level of the LSM tree.
: 
: The same about BPS tree extents. LSM 0 level is a BPS tree, whose
: blocks are allocated on lsregion. The extents are used as pointer
: arrays inside the tree, so they need alignof(void *) alignment.
: 
: The mentioned unaligned accesses were revealed by clang undefined
: behaviour sanitizer, and are fixed by this patch.
: 
: Part of #4609
: ---
:  src/box/vy_mem.c                |  9 +++++----
:  src/box/vy_stmt.c               | 13 ++++++++-----
:  test/vinyl/quota.result         | 10 +++++-----
:  test/vinyl/quota_timeout.result |  4 ++--
:  test/vinyl/stat.result          |  4 ++--
:  5 files changed, 22 insertions(+), 18 deletions(-)
: 
: diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
: index b4d016a68..98027e784 100644
: --- a/src/box/vy_mem.c
: +++ b/src/box/vy_mem.c
: @@ -75,11 +75,12 @@ vy_mem_tree_extent_alloc(void *ctx)
:  {
:  	struct vy_mem *mem = (struct vy_mem *) ctx;
:  	struct vy_mem_env *env = mem->env;
: -	void *ret = lsregion_alloc(&env->allocator, VY_MEM_TREE_EXTENT_SIZE,
: -				   mem->generation);
: +	void *ret = lsregion_aligned_alloc(&env->allocator,
: +					   VY_MEM_TREE_EXTENT_SIZE,
: +					   alignof(void *), mem->generation);
:  	if (ret == NULL) {
: -		diag_set(OutOfMemory, VY_MEM_TREE_EXTENT_SIZE,
: "lsregion_alloc",
: -			 "ret");
: +		diag_set(OutOfMemory, VY_MEM_TREE_EXTENT_SIZE,
: +			 "lsregion_aligned_alloc", "ret");
:  		return NULL;
:  	}
:  	mem->tree_extent_size += VY_MEM_TREE_EXTENT_SIZE;
: diff --git a/src/box/vy_stmt.c b/src/box/vy_stmt.c
: index adc3ba452..dc6960068 100644
: --- a/src/box/vy_stmt.c
: +++ b/src/box/vy_stmt.c
: @@ -223,20 +223,23 @@ vy_stmt_dup_lsregion(struct tuple *stmt, struct
: lsregion *lsregion,
:  	size_t size = tuple_size(stmt);
:  	size_t alloc_size = size;
:  	struct tuple *mem_stmt;
: +	const size_t align = alignof(struct vy_stmt);
: 
:  	/* Reserve one byte for UPSERT counter. */
:  	if (type == IPROTO_UPSERT)
: -		alloc_size++;
: +		alloc_size += align;
: 
: -	mem_stmt = lsregion_alloc(lsregion, alloc_size, alloc_id);
: +	mem_stmt = lsregion_aligned_alloc(lsregion, alloc_size, align,
: +					  alloc_id);
:  	if (mem_stmt == NULL) {
: -		diag_set(OutOfMemory, size, "lsregion_alloc", "mem_stmt");
: +		diag_set(OutOfMemory, size, "lsregion_aligned_alloc",
: +			 "mem_stmt");
:  		return NULL;
:  	}
: 
:  	if (type == IPROTO_UPSERT) {
: -		*(uint8_t *)mem_stmt = 0;
: -		mem_stmt = (struct tuple *)((uint8_t *)mem_stmt + 1);
: +		memset(mem_stmt, 0, align);
: +		mem_stmt = (struct tuple *)((uint8_t *)mem_stmt + align);
:  	}
: 
:  	memcpy(mem_stmt, stmt, size);
: diff --git a/test/vinyl/quota.result b/test/vinyl/quota.result
: index d1b28ee51..940df4e49 100644
: --- a/test/vinyl/quota.result
: +++ b/test/vinyl/quota.result
: @@ -31,7 +31,7 @@ space:insert({1, 1})
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 98343
: +- 98344
:  ...
:  space:insert({1, 1})
:  ---
: @@ -39,7 +39,7 @@ space:insert({1, 1})
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 98343
: +- 98344
:  ...
:  space:update({1}, {{'!', 1, 100}}) -- try to modify the primary key
:  ---
: @@ -47,7 +47,7 @@ space:update({1}, {{'!', 1, 100}}) -- try to modify the
: primary key
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 98343
: +- 98344
:  ...
:  space:insert({2, 2})
:  ---
: @@ -63,7 +63,7 @@ space:insert({4, 4})
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 98460
: +- 98463
:  ...
:  box.snapshot()
:  ---
: @@ -89,7 +89,7 @@ _ = space:replace{1, 1, string.rep('a', 1024 * 1024 *
: 5)}
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 5292076
: +- 5292080
:  ...
:  space:drop()
:  ---
: diff --git a/test/vinyl/quota_timeout.result
: b/test/vinyl/quota_timeout.result
: index 7a71b29c6..31ca23670 100644
: --- a/test/vinyl/quota_timeout.result
: +++ b/test/vinyl/quota_timeout.result
: @@ -49,7 +49,7 @@ s:count()
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 748241
: +- 748248
:  ...
:  -- Since the following operation requires more memory than configured
:  -- and dump is disabled, it should fail with ER_VY_QUOTA_TIMEOUT.
: @@ -63,7 +63,7 @@ s:count()
:  ...
:  box.stat.vinyl().memory.level0
:  ---
: -- 748241
: +- 748248
:  ...
:  --
:  -- Check that increasing box.cfg.vinyl_memory wakes up fibers
: diff --git a/test/vinyl/stat.result b/test/vinyl/stat.result
: index d35def13d..a895528b9 100644
: --- a/test/vinyl/stat.result
: +++ b/test/vinyl/stat.result
: @@ -761,7 +761,7 @@ put(1)
:  ...
:  stat_diff(gstat(), st, 'memory.level0')
:  ---
: -- 1061
: +- 1064
:  ...
:  -- use cache
:  st = gstat()
: @@ -1130,7 +1130,7 @@ gstat()
:    memory:
:      tuple_cache: 14417
:      tx: 0
: -    level0: 262583
: +    level0: 263210
:      page_index: 1250
:      bloom_filter: 140
:    disk:
: --
: 2.21.1 (Apple Git-122.3)