chewbranca commented on a change in pull request #610: Optimize ddoc cache URL: https://github.com/apache/couchdb/pull/610#discussion_r125151857
########## File path: src/ddoc_cache/src/ddoc_cache_lru.erl ########## @@ -0,0 +1,293 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(ddoc_cache_lru). +-behaviour(gen_server). +-vsn(1). + + +-export([ + start_link/0, + open/1, + refresh/2 +]). + +-export([ + init/1, + terminate/2, + handle_call/3, + handle_cast/2, + handle_info/2, + code_change/3 +]). + +-export([ + handle_db_event/3 +]). + + +-include("ddoc_cache.hrl"). + + +-record(st, { + pids, % pid -> key + dbs, % dbname -> docid -> key -> pid + size, + evictor +}). + + +start_link() -> + gen_server:start_link({local, ?MODULE}, ?MODULE, [], []). + + +open(Key) -> + try ets:lookup(?CACHE, Key) of Review comment: In `ddoc_cache:open_doc/{2,3}` the `Key` is `{ddoc_cache_entry_ddocid, {DbName, DocId}}` and `{ddoc_cache_entry_ddocid_rev, {DbName, DocId, RevId}}`, respectively. That implies we'll have separate pids for the "latest version" and "specific versions" cases. Is that intentional? It also means we'll end up triggering two ddoc_cache load cycles on an uncached ddoc view request. For instance in `fabric:query_view/6` [1] we call the two arity `open_doc` giving us `Key = {ddoc_cache_entry_ddocid, {DbName, DocId}}`. We then extract the doc_id/rev pair [2] to ensure all view shards use the same ddoc and ship that in the rpc calls, at which point we use the three arity `open_doc` variant to open the specific ddoc rev [3]. This means we'll perform a non ddoc-rev cache fetch on the coordinator, followed by up to Q*N ddoc-rev cache lookups on the rpc nodes, leaving the coordinator node with the ddoc entry and no ddoc-rev entry, and the rpc nodes with the ddoc-rev entry and no ddoc entry. Then we'll potentially perform a non ddoc-rev fetch on every single coordinator node until every node has the non ddoc-rev cache entry, even if they already have the latest ddoc-rev entry. So if you have 12 nodes, and round robin one view request across all of them you'll block on c aching the non ddoc-rev version 12 times, despite most likely already having the relevant latest rev in the ddoc-rev version. Perhaps this isn't the end of the world, but it definitely seems awkward and potentially avoidable. In the case of loading the ddoc-rev version you could insert a `?CACHE` entry for the non ddoc-rev version pointing to that pid, but that might be awkward in the inverse case when you start with `ddoc_open/2`. On a related note, does this mean that we'll have pids sticking around for old ddoc rev versions until they're evicted from the cache? [1] https://github.com/apache/couchdb/blob/master/src/fabric/src/fabric.erl#L337 [2] https://github.com/apache/couchdb/blob/master/src/fabric/src/fabric_view_map.erl#L28 [3] https://github.com/apache/couchdb/blob/master/src/fabric/src/fabric_rpc.erl#L119 ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
