Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] net.box: drop conn:timeout()
Date: Tue, 27 Jul 2021 13:06:16 +0300	[thread overview]
Message-ID: <20210727100616.qjnhzfiggwqo2uag@esperanza> (raw)
In-Reply-To: <e4874f7e-562f-9862-b519-ed5f23eae845@tarantool.org>

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()

  reply	other threads:[~2021-07-27 10:06 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-26 10:58 Vladimir Davydov via Tarantool-patches
2021-07-26 22:37 ` Vladislav Shpilevoy via Tarantool-patches
2021-07-27 10:06   ` Vladimir Davydov via Tarantool-patches [this message]
2021-07-27 21:27     ` Vladislav Shpilevoy via Tarantool-patches
2021-07-29  8:05     ` Vladimir Davydov via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210727100616.qjnhzfiggwqo2uag@esperanza \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=vdavydov@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] net.box: drop conn:timeout()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox