Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Couch server improvements #1593

Merged
merged 2 commits into from Sep 12, 2018
Merged

Couch server improvements #1593

merged 2 commits into from Sep 12, 2018

Conversation

@chewbranca
Copy link
Contributor

@chewbranca chewbranca commented Sep 4, 2018

Overview

This PR provides two performance related improvements to couch_server:

  1. Use {read_concurrency, true} for the protected couch_dbs ETS table used in couch_server.
  2. Don't send update_lru messages to couch_server when disabled

More details in the corresponding commit messages.

Testing recommendations

Standard test suite should do the trick.

Related Issues or Pull Requests

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
case lists:member(sys_db, Options) of
false -> gen_server:cast(couch_server, {update_lru, DbName});
true -> ok
case config:get_boolean("couchdb", "update_lru_on_read", false) of

This comment has been minimized.

@iilyak

iilyak Sep 4, 2018
Contributor

This would add one extra ets:lookup here (which we probably can accept). A hacky alternative could be to extend #entry record so we can get the value of the parameter before sending message to couch_server.

This comment has been minimized.

@chewbranca

chewbranca Sep 4, 2018
Author Contributor

Yeah... I try to pretend that ETS lookup is fast enough to not be relevant. We do config lookups all over the place, so if we're concerned about that I think we need to deal with that as a bigger problem than working around individual lookups. For instance, one option would be to bootstrap new http request pids with a prebuilt config table that has all the options they need to lookup so no concurrent ETS interactions are necessary. Although we would need to do a fair bit of testing to actually determine whether config lookups are a bottleneck.

This comment has been minimized.

@nickva

nickva Sep 7, 2018
Contributor

We could do the mochiglobal pattern eventually.

But last time I had worried about ets:lookup I had measured it at less than 5 microseconds.

Was curious and tried it again my mac laptop:

f(Tries), f(T), Tries = lists:seq(1,1000000), {T, ok} = timer:tc(fun() -> lists:foreach( fun(_) -> config:get_boolean("couchdb", "update_lru_on_read", false) end, Tries) end), T/1.0e6.
0.955564

On a linux server:

f(Tries), f(T), Tries = lists:seq(1,1000000), {T, ok} = timer:tc(fun() -> lists:foreach( fun(_) -> config:get_boolean("couchdb", "update_lru_on_read", false) end, Tries) end), T/1.0e6.
1.264781

So seems pretty small. Granted neither the laptop not the server were very busy, so maybe with a larger number of lock acquisitions it looks much worse.

@davisp
Copy link
Member

@davisp davisp commented Sep 6, 2018

Another thing we should consider is sending the Timeout value in the open messages so that couch_server can discard any requests from a client that's already wandered off.

Timeout = couch_util:get_value(timeout, Options, infinity),

@davisp
Copy link
Member

@davisp davisp commented Sep 6, 2018

And by timeout, I mean we should send an expiry in the message. So something like "now + Timestamp" and then couch_server can throw it away if that's in the past.

protected,
named_table,
{keypos, #entry.name},
{read_concurrency, true}

This comment has been minimized.

@nickva

nickva Sep 7, 2018
Contributor

Wonder if this will slow down updates to the table. I forgot what the trade-offs for read_concurrency are...

This comment has been minimized.

@davisp

davisp Sep 10, 2018
Member

If memory serves, its the flip between read and write requests which is expensive which we already do exhaustively so I don't think it'd be that big of a deal.

@davisp
Copy link
Member

@davisp davisp commented Sep 10, 2018

Here's a quick stab at a deadline implementation:

d2bf663

chewbranca added 2 commits Sep 4, 2018
This adds the read_concurrency option to couch_server's ETS table for
couch_dbs which contains the references to open database handles. This
is an obvious improvement as all callers opening database pids interact
with this ETS table concurrently. Conversely, the couch_server pid is
the only writer, so no need for write_concurrency.
The couchdb.update_lru_on_read setting controls whether couch_server
uses read requests as LRU update triggers. Unfortunately, the messages
for update_lru on reads are sent regardless of whether this is enabled
or disabled. While in principle this is harmless, and overloaded
couch_server pid can accumulate a considerable volume of these messages,
even when disabled. This patch prevents the caller from sending an
update_lru message when the setting is disabled.
@chewbranca chewbranca force-pushed the couch-server-improvements branch from 4afed4e to 79dd054 Sep 11, 2018
@davisp
Copy link
Member

@davisp davisp commented Sep 12, 2018

+1 to merge ignoring my deadline commit

@chewbranca chewbranca merged commit d56c728 into master Sep 12, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nickva nickva deleted the couch-server-improvements branch Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants