Re: [Tarantool-patches] [PATCH v4 6/9] net.box: add stream support to net.box

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 8f5671c15..8d707fb26 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua

<...>

> +-- Metatable for stream space indexes. When stream space being
> +-- created there are no indexes in it. When accessing the space
> +-- index, we look for corresponding space index in corresponding
> +-- connection space. If it is found we create same index for the
> +-- stream space but with corresponding stream ID. We do not need
> +-- to compare stream _schema_version and connection schema_version,
> +-- because all access to index  is carried out through it's space.
> +-- So we update schema_version when we access space.
> +local stream_indexes_mt = {
> +    __index = function(self, key)
> +        local _space = self._space
> +        local src = _space._src.index[key]
> +        if not src then
> +            return nil
> +        end
> +        local res = stream_wrap_index(_space._stream_id, src)
> +        self[key] = res
> +        return res
> +    end,
> +    __serialize = stream_indexes_serialize
> +}
> +
> +-- Create stream space, which is same as connection space,
> +-- but have non zero stream ID.
> +local function stream_wrap_space(stream, src)
> +    local res = setmetatable({
> +        _stream_id = stream._stream_id,
> +        _src = src,
> +        index = setmetatable({
> +            _space = nil,
> +        }, stream_indexes_mt)
> +    }, {
> +        __index = src,
> +        __serialize = stream_space_serialize
> +    })

1. That is the thing I explicitly tried to avoid when proposed
to make a light proxy of conn spaces - it is not good to create
a new metatable for each space. Can you create it one time and
use it for all space objects? Like stream_indexes_mt - it is
shared by all 'index hashes'.

The only reason you create the metatable here is to use 'src'
as __index, but it might be done with a function.

	__index = function(self, key)
		return self._src[key]
	end

And it does not need to be a closure. This should make possible
to share the metatable by all spaces.

The same for stream metatable in :new_stream().

> +    res.index._space = res
> +    return res
> +end
> +
> +-- Metatable for stream spaces. When stream being created there
> +-- are no spaces in it. When user try to access some space in
> +-- stream, we first of all compare _schema_version of stream with
> +-- schema_version from connection and if they are not equal, we
> +-- clear stream space cache and update it's schema_version. Then
> +-- we look for corresponding space in the connection. If it is
> +-- found we create same space for the stream but with corresponding
> +-- stream ID.
> +local stream_spaces_mt = {
> +    __index = function(self, key)
> +        local stream = self._stream
> +        if stream._schema_version ~= stream._conn.schema_version then
> +            stream._schema_version = stream._conn.schema_version
> +            self._stream_space_cache = {}
> +        end
> +        if self._stream_space_cache[key] then
> +            return self._stream_space_cache[key]
> +        end
> +        local src = stream._conn.space[key]

2. Please, try to cache 'stream._conn' into a local variable so as
not to look it up more than once ('.' operator is not free).

The same for stream._stream_space_cache.

The same for stream._conn.schema_version.

The same for _last_stream_id in :new_stream().

> +        if not src then
> +            return nil
> +        end
> +        local res = stream_wrap_space(stream, src)
> +        self._stream_space_cache[key] = res
> +        return res
> +    end,
> +    __serialize = stream_spaces_serialize
> +}