conn:timeout(timeout) writes (now + timeout) to conn._deadlines table keyed by fiber.self(). The deadline will then be used by all conn methods unless specified explicitly in the options argument. Usage of fiber.self() degrades net.box performance by about 15% in case timeout is not specified, because it is a C call and so interferes with JIT. See the ticket for the test details. Since the feature is unusable and was deprecated more than four years ago (in 1.7.4) in favor of specifying timeout in the options argument (see commit 58da206c33197), it should be fine to drop it. Closes #6242 --- .../gh-6242-net-box-drop-conn-timeout.md | 9 ++++ src/box/lua/net_box.lua | 26 ---------- .../net.box_count_inconsistent_gh-3262.result | 49 ------------------- ...et.box_count_inconsistent_gh-3262.test.lua | 20 -------- 4 files changed, 9 insertions(+), 95 deletions(-) create mode 100644 changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md diff --git a/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md b/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md new file mode 100644 index 000000000000..69d12fb858e5 --- /dev/null +++ b/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md @@ -0,0 +1,9 @@ +## feature/core + +* **[Breaking change]** timeout() method of net.box connection, which was + marked deprecated more than four years ago (in 1.7.4) was dropped, because + it negatively affected performance of hot net.box methods, like call() and + select(), in case those are called without specifying a timeout. + +--- +Breaking change: timeout() method of net.box connection was dropped. diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index 3878abf21914..15e6c093cc15 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -1047,9 +1047,6 @@ local function new_sm(host, port, opts, connection, greeting) setmetatable(remote, console_mt) else setmetatable(remote, remote_mt) - -- @deprecated since 1.7.4 - remote._deadlines = setmetatable({}, {__mode = 'k'}) - remote._space_mt = space_metatable(remote) remote._index_mt = index_metatable(remote) if opts.call_16 then @@ -1176,17 +1173,11 @@ function remote_methods:_request(method, opts, request_ctx, ...) return res end if opts.timeout then - -- conn.space:request(, { timeout = timeout }) deadline = fiber_clock() + opts.timeout - else - -- conn:timeout(timeout).space:request() - -- @deprecated since 1.7.4 - deadline = self._deadlines[fiber_self()] end on_push = opts.on_push or on_push_sync_default on_push_ctx = opts.on_push_ctx else - deadline = self._deadlines[fiber_self()] on_push = on_push_sync_default end -- Execute synchronous request. @@ -1289,28 +1280,11 @@ end function remote_methods:wait_state(state, timeout) check_remote_arg(self, 'wait_state') - if timeout == nil then - local deadline = self._deadlines[fiber_self()] - timeout = deadline and max(0, deadline - fiber_clock()) - end return self._transport.wait_state(state, timeout) end local compat_warning_said = false --- @deprecated since 1.7.4 -function remote_methods:timeout(timeout) - check_remote_arg(self, 'timeout') - if not compat_warning_said then - compat_warning_said = true - log.warn("netbox:timeout(timeout) is deprecated since 1.7.4, ".. - "please use space:<request>(..., {timeout = t}) instead.") - end - -- Sic: this is broken by design - self._deadlines[fiber_self()] = (timeout and fiber_clock() + timeout) - return self -end - function remote_methods:_install_schema(schema_version, spaces, indices, collations) local sl, space_mt, index_mt = {}, self._space_mt, self._index_mt diff --git a/test/box/net.box_count_inconsistent_gh-3262.result b/test/box/net.box_count_inconsistent_gh-3262.result index c77677625d97..286f9ded8aea 100644 --- a/test/box/net.box_count_inconsistent_gh-3262.result +++ b/test/box/net.box_count_inconsistent_gh-3262.result @@ -427,55 +427,6 @@ cn:eval('return ret_after(...)', {1.00}, { timeout = 1e-9 }) --- - error: Timeout exceeded ... --- --- :timeout() --- @deprecated since 1.7.4 --- -cn:timeout(1).space.net_box_test_space.index.primary:select{234} ---- -- - [234, 1, 2, 3] -... -cn:call('ret_after', {.01}) ---- -- [[0.01]] -... -cn:timeout(1):call('ret_after', {.01}) ---- -- [[0.01]] -... -cn:timeout(.01):call('ret_after', {1}) ---- -- error: Timeout exceeded -... -cn = remote:timeout(0.0000000001):connect(LISTEN.host, LISTEN.service, { user = 'netbox', password = '123' }) ---- -... -cn:close() ---- -... -cn = remote:timeout(1):connect(LISTEN.host, LISTEN.service, { user = 'netbox', password = '123' }) ---- -... -remote.self:ping() ---- -- true -... -remote.self.space.net_box_test_space:select{234} ---- -- - [234, 1, 2, 3] -... -remote.self:timeout(123).space.net_box_test_space:select{234} ---- -- - [234, 1, 2, 3] -... -remote.self:is_connected() ---- -- true -... -remote.self:wait_connected() ---- -- true -... cn:close() --- ... diff --git a/test/box/net.box_count_inconsistent_gh-3262.test.lua b/test/box/net.box_count_inconsistent_gh-3262.test.lua index e84e85cf3829..a6d87cdcc830 100644 --- a/test/box/net.box_count_inconsistent_gh-3262.test.lua +++ b/test/box/net.box_count_inconsistent_gh-3262.test.lua @@ -155,26 +155,6 @@ cn:call('ret_after', {1.00}, { timeout = 1e-9 }) cn:eval('return ret_after(...)', {0.01}, { timeout = 1.00 }) cn:eval('return ret_after(...)', {1.00}, { timeout = 1e-9 }) --- --- :timeout() --- @deprecated since 1.7.4 --- - -cn:timeout(1).space.net_box_test_space.index.primary:select{234} -cn:call('ret_after', {.01}) -cn:timeout(1):call('ret_after', {.01}) -cn:timeout(.01):call('ret_after', {1}) - -cn = remote:timeout(0.0000000001):connect(LISTEN.host, LISTEN.service, { user = 'netbox', password = '123' }) -cn:close() -cn = remote:timeout(1):connect(LISTEN.host, LISTEN.service, { user = 'netbox', password = '123' }) - -remote.self:ping() -remote.self.space.net_box_test_space:select{234} -remote.self:timeout(123).space.net_box_test_space:select{234} -remote.self:is_connected() -remote.self:wait_connected() - cn:close() -- cleanup database after tests space:drop() -- 2.25.1
Hi! Thanks for the patch! I hope I was supposed to review this. Could you please add the reviewers into CC/To for next patches? See 5 comments below. On 26.07.2021 12:58, Vladimir Davydov via Tarantool-patches wrote: > conn:timeout(timeout) writes (now + timeout) to conn._deadlines table > keyed by fiber.self(). The deadline will then be used by all conn > methods unless specified explicitly in the options argument. > > Usage of fiber.self() degrades net.box performance by about 15% in case > timeout is not specified, because it is a C call and so interferes with > JIT. See the ticket for the test details. > > Since the feature is unusable and was deprecated more than four years > ago (in 1.7.4) in favor of specifying timeout in the options argument > (see commit 58da206c33197), it should be fine to drop it. 1. Is it possible to add the commit title after the hash in quotes and parentheses? Like this: (see commit 58da206c33197 ("net.box: add { timeout = } option to requests")) It helps to quickly understand what was the commit about without copy-pasting the hash for `git show`. https://github.com/tarantool/tarantool/wiki/Code-review-procedure#commit-message > Closes #6242 > --- 2. May I ask you please to push the patch onto a branch and attach the issue and branch links after `---` in the first/cover email? For next patches and for this one too. It simplifies the review; makes your patch pass CI before the review (or not pass); and simplifies patch push in the end. https://github.com/tarantool/tarantool/wiki/Code-review-procedure#sending-the-patch 3. The "feature" is still documented: https://www.tarantool.io/en/doc/latest/reference/reference_lua/net_box/#conn-timeout You need to add a docbot request to the commit message for the documentation team to remove all the mentionings of the old API. For the syntax you can look at https://github.com/tarantool/docbot#docbot---tarantool-documentation-pipeline-bot. For examples: grep by `TarantoolBot` in git log. > diff --git a/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md b/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md > new file mode 100644 > index 000000000000..69d12fb858e5 > --- /dev/null > +++ b/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md > @@ -0,0 +1,9 @@ > +## feature/core > + > +* **[Breaking change]** timeout() method of net.box connection, which was > + marked deprecated more than four years ago (in 1.7.4) was dropped, because > + it negatively affected performance of hot net.box methods, like call() and > + select(), in case those are called without specifying a timeout. 4. Wouldn't it be hard, please, to reference the issue in the end of the text using `(gh-####)` format? See other changelogs for examples. > + > +--- > +Breaking change: timeout() method of net.box connection was dropped. > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua > index 3878abf21914..15e6c093cc15 100644 > --- a/src/box/lua/net_box.lua > +++ b/src/box/lua/net_box.lua > @@ -1289,28 +1280,11 @@ end > > function remote_methods:wait_state(state, timeout) > check_remote_arg(self, 'wait_state') > - if timeout == nil then > - local deadline = self._deadlines[fiber_self()] > - timeout = deadline and max(0, deadline - fiber_clock()) > - end > return self._transport.wait_state(state, timeout) > end > > local compat_warning_said = false > > --- @deprecated since 1.7.4 > -function remote_methods:timeout(timeout) > - check_remote_arg(self, 'timeout') > - if not compat_warning_said then 5. compat_warning_said can be deleted now. In the future you can find such remnants using luacheck, which was integrated into our CI and can be run locally. For that you need to install Tarantool into any folder, install luacheck rock, and then you can do something like this: $ ./.rocks/bin/luacheck --codes --config .luacheckrc src/box/lua/net_box.lua Checking src/box/lua/net_box.lua 1 warning src/box/lua/net_box.lua:1286:7: (W211) unused variable compat_warning_said
On Tue, Jul 27, 2021 at 12:37:17AM +0200, Vladislav Shpilevoy wrote: > See 5 comments below. All fixed, thanks. The patch is below: https://github.com/tarantool/tarantool/tree/vdavydov/net-box-drop-conn-timeout From f1dbd33e4ccbc586d12460b9061c4c6a30b95f32 Mon Sep 17 00:00:00 2001 From: Vladimir Davydov <vdavydov@tarantool.org> Date: Mon, 26 Jul 2021 11:59:15 +0300 Subject: [PATCH] net.box: drop conn:timeout() conn:timeout(timeout) writes (now + timeout) to conn._deadlines table keyed by fiber.self(). The deadline will then be used by all conn methods unless specified explicitly in the options argument. Usage of fiber.self() degrades net.box performance by about 15% in case timeout is not specified, because it is a C call and so interferes with JIT. See the ticket for the test details. Since the feature is unusable and was deprecated more than four years ago (in 1.7.4) in favor of specifying timeout in the options argument (see commit 58da206c33197 ("net.box: add { timeout = } option to requests")), it should be fine to drop it. Closes #6242 @TarantoolBot document Title: Delete documentation entry for net.box conn:timeout() conn:timeout() was deprecated more than four years ago (in 1.7.4). Since it results in performance degradation in case timeout is not specified explicitly in request options, we decided to drop it. Please remove it from the documentation as well: https://www.tarantool.io/en/doc/latest/reference/reference_lua/net_box/#conn-timeout diff --git a/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md b/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md new file mode 100644 index 000000000000..9571c0afc577 --- /dev/null +++ b/changelogs/unreleased/gh-6242-net-box-drop-conn-timeout.md @@ -0,0 +1,9 @@ +## feature/core + +* **[Breaking change]** timeout() method of net.box connection, which was + marked deprecated more than four years ago (in 1.7.4) was dropped, because + it negatively affected performance of hot net.box methods, like call() and + select(), in case those are called without specifying a timeout (gh-6242). + +--- +Breaking change: timeout() method of net.box connection was dropped. diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index 3878abf21914..d70927989b71 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -1047,9 +1047,6 @@ local function new_sm(host, port, opts, connection, greeting) setmetatable(remote, console_mt) else setmetatable(remote, remote_mt) - -- @deprecated since 1.7.4 - remote._deadlines = setmetatable({}, {__mode = 'k'}) - remote._space_mt = space_metatable(remote) remote._index_mt = index_metatable(remote) if opts.call_16 then @@ -1176,17 +1173,11 @@ function remote_methods:_request(method, opts, request_ctx, ...) return res end if opts.timeout then - -- conn.space:request(, { timeout = timeout }) deadline = fiber_clock() + opts.timeout - else - -- conn:timeout(timeout).space:request() - -- @deprecated since 1.7.4 - deadline = self._deadlines[fiber_self()] end on_push = opts.on_push or on_push_sync_default on_push_ctx = opts.on_push_ctx else - deadline = self._deadlines[fiber_self()] on_push = on_push_sync_default end -- Execute synchronous request. @@ -1289,28 +1280,9 @@ end function remote_methods:wait_state(state, timeout) check_remote_arg(self, 'wait_state') - if timeout == nil then - local deadline = self._deadlines[fiber_self()] - timeout = deadline and max(0, deadline - fiber_clock()) - end return self._transport.wait_state(state, timeout) end -local compat_warning_said = false - --- @deprecated since 1.7.4 -function remote_methods:timeout(timeout) - check_remote_arg(self, 'timeout') - if not compat_warning_said then - compat_warning_said = true - log.warn("netbox:timeout(timeout) is deprecated since 1.7.4, ".. - "please use space:<request>(..., {timeout = t}) instead.") - end - -- Sic: this is broken by design - self._deadlines[fiber_self()] = (timeout and fiber_clock() + timeout) - return self -end - function remote_methods:_install_schema(schema_version, spaces, indices, collations) local sl, space_mt, index_mt = {}, self._space_mt, self._index_mt diff --git a/test/box/net.box_count_inconsistent_gh-3262.result b/test/box/net.box_count_inconsistent_gh-3262.result index c77677625d97..286f9ded8aea 100644 --- a/test/box/net.box_count_inconsistent_gh-3262.result +++ b/test/box/net.box_count_inconsistent_gh-3262.result @@ -427,55 +427,6 @@ cn:eval('return ret_after(...)', {1.00}, { timeout = 1e-9 }) --- - error: Timeout exceeded ... --- --- :timeout() --- @deprecated since 1.7.4 --- -cn:timeout(1).space.net_box_test_space.index.primary:select{234} ---- -- - [234, 1, 2, 3] -... -cn:call('ret_after', {.01}) ---- -- [[0.01]] -... -cn:timeout(1):call('ret_after', {.01}) ---- -- [[0.01]] -... -cn:timeout(.01):call('ret_after', {1}) ---- -- error: Timeout exceeded -... -cn = remote:timeout(0.0000000001):connect(LISTEN.host, LISTEN.service, { user = 'netbox', password = '123' }) ---- -... -cn:close() ---- -... -cn = remote:timeout(1):connect(LISTEN.host, LISTEN.service, { user = 'netbox', password = '123' }) ---- -... -remote.self:ping() ---- -- true -... -remote.self.space.net_box_test_space:select{234} ---- -- - [234, 1, 2, 3] -... -remote.self:timeout(123).space.net_box_test_space:select{234} ---- -- - [234, 1, 2, 3] -... -remote.self:is_connected() ---- -- true -... -remote.self:wait_connected() ---- -- true -... cn:close() --- ... diff --git a/test/box/net.box_count_inconsistent_gh-3262.test.lua b/test/box/net.box_count_inconsistent_gh-3262.test.lua index e84e85cf3829..a6d87cdcc830 100644 --- a/test/box/net.box_count_inconsistent_gh-3262.test.lua +++ b/test/box/net.box_count_inconsistent_gh-3262.test.lua @@ -155,26 +155,6 @@ cn:call('ret_after', {1.00}, { timeout = 1e-9 }) cn:eval('return ret_after(...)', {0.01}, { timeout = 1.00 }) cn:eval('return ret_after(...)', {1.00}, { timeout = 1e-9 }) --- --- :timeout() --- @deprecated since 1.7.4 --- - -cn:timeout(1).space.net_box_test_space.index.primary:select{234} -cn:call('ret_after', {.01}) -cn:timeout(1):call('ret_after', {.01}) -cn:timeout(.01):call('ret_after', {1}) - -cn = remote:timeout(0.0000000001):connect(LISTEN.host, LISTEN.service, { user = 'netbox', password = '123' }) -cn:close() -cn = remote:timeout(1):connect(LISTEN.host, LISTEN.service, { user = 'netbox', password = '123' }) - -remote.self:ping() -remote.self.space.net_box_test_space:select{234} -remote.self:timeout(123).space.net_box_test_space:select{234} -remote.self:is_connected() -remote.self:wait_connected() - cn:close() -- cleanup database after tests space:drop()
Hi! Thanks for the fixes! LGTM.
Pushed to master.