You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "nickva (via GitHub)" <gi...@apache.org> on 2024/08/02 22:52:20 UTC
[PR] Fix and improve couch_lru [couchdb]
nickva opened a new pull request, #5165:
URL: https://github.com/apache/couchdb/pull/5165
Previously, `couch_lru` could crash with a ```function_clause gb_trees:delete_1(...,nil)``` error, indicating that we tried to remove an non-existent entry from the tree. That happened because we were updating the cache, which updated the tree object from it, but traversing iterator was not updated. As a result the two trees got out of "sync" and entries we were expecting to be there were now gone.
The solution, as suggested by Russell, is to just create a new iterator if from the updated cache.
We further observed that as a result of this fix, the iterator itself became un-necessary, since every path through the function updates the tree, so required creating a new iterator. As a result it makes sense to just fetch the next key value pair with [larger/2](https://www.erlang.org/doc/apps/stdlib/gb_trees.html#larger/2). Because `larger/2` is present in OTP 27+ only, create local compat function until then.
Also take the opportunity to replace the dict with maps, to hopefully get a gain a bit of performance.
Co-author: Russell Branca
Co-author: Robert Newson
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #5165:
URL: https://github.com/apache/couchdb/pull/5165#issuecomment-2267178171
Closing the PR for now, this approach won't work, I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5165:
URL: https://github.com/apache/couchdb/pull/5165#discussion_r1702884283
##########
src/couch/src/couch_lru.erl:
##########
@@ -16,33 +16,34 @@
-include("couch_server_int.hrl").
new() ->
- {gb_trees:empty(), dict:new()}.
+ % {gb_trees(UniqueInt -> DbName), #{DbName => UniqueInt}}
+ {gb_trees:empty(), #{}}.
-insert(DbName, {Tree0, Dict0}) ->
+insert(DbName, {Tree0, #{} = Dict0}) ->
Review Comment:
That would work, good idea. I thought about it, but couldn't decide thinking a "dict" is technically not incorrect in the general sense. But, yeah it would be less confusing if the name matched the data type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #5165:
URL: https://github.com/apache/couchdb/pull/5165#issuecomment-2266236454
Draft PR for now, needs some tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5165:
URL: https://github.com/apache/couchdb/pull/5165#discussion_r1703051667
##########
src/couch/src/couch_lru.erl:
##########
@@ -16,33 +16,34 @@
-include("couch_server_int.hrl").
new() ->
- {gb_trees:empty(), dict:new()}.
+ % {gb_trees(UniqueInt -> DbName), #{DbName => UniqueInt}}
+ {gb_trees:empty(), #{}}.
-insert(DbName, {Tree0, Dict0}) ->
+insert(DbName, {Tree0, #{} = Dict0}) ->
Review Comment:
I did so in this PR: https://github.com/apache/couchdb/pull/5168
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva closed pull request #5165: Fix and improve couch_lru
URL: https://github.com/apache/couchdb/pull/5165
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5165:
URL: https://github.com/apache/couchdb/pull/5165#discussion_r1702884283
##########
src/couch/src/couch_lru.erl:
##########
@@ -16,33 +16,34 @@
-include("couch_server_int.hrl").
new() ->
- {gb_trees:empty(), dict:new()}.
+ % {gb_trees(UniqueInt -> DbName), #{DbName => UniqueInt}}
+ {gb_trees:empty(), #{}}.
-insert(DbName, {Tree0, Dict0}) ->
+insert(DbName, {Tree0, #{} = Dict0}) ->
Review Comment:
That would work, good idea. I thought about it, but couldn't decide thinking a "dict" is technically is not incorrect in the general sense. But, yeah it would be less confusing if the name matched the data type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5165:
URL: https://github.com/apache/couchdb/pull/5165#discussion_r1702920150
##########
src/couch/src/couch_lru.erl:
##########
@@ -16,33 +16,34 @@
-include("couch_server_int.hrl").
new() ->
- {gb_trees:empty(), dict:new()}.
+ % {gb_trees(UniqueInt -> DbName), #{DbName => UniqueInt}}
+ {gb_trees:empty(), #{}}.
-insert(DbName, {Tree0, Dict0}) ->
+insert(DbName, {Tree0, #{} = Dict0}) ->
Review Comment:
I think I'll just create a separate PR which updates the dict to map only and also adds some units tests. This way we'll separate the two things and if we make any logic LRU updates we'll have tests to run at least.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5165:
URL: https://github.com/apache/couchdb/pull/5165#discussion_r1702393733
##########
src/couch/src/couch_lru.erl:
##########
@@ -53,14 +54,19 @@ close_int({Lru, DbName, Iter}, {Tree, Dict} = Cache) ->
true = ets:delete(CouchDbs, DbName),
true = ets:delete(CouchDbsPidToName, Pid),
exit(Pid, kill),
- {true, {gb_trees:delete(Lru, Tree), dict:erase(DbName, Dict)}};
+ {true, {gb_trees:delete(Lru, Tree), maps:remove(DbName, Dict)}};
false ->
true = couch_server:unlock(CouchDbs, DbName),
couch_stats:increment_counter([couchdb, couch_server, lru_skip]),
- close_int(gb_trees:next(Iter), update(DbName, Cache))
+ NewCache = {NewTree, _} = update(DbName, Cache),
+ close_int(larger(Lru, NewTree), NewCache)
Review Comment:
This fixes trees getting out of sync, but gets us in another trouble: non-termination. If all the entries are not idle, then we'd be bumping them up as we see them. The `Lru` key would increase, for example, from 5 to 11, then we see the key again 11, and we bump it again, now it's at say 13, we bump it so on.
We'll have to figure something else out here. We've been discussing batching the entries needing an update for after close_int returns perhaps....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #5165:
URL: https://github.com/apache/couchdb/pull/5165#discussion_r1702866106
##########
src/couch/src/couch_lru.erl:
##########
@@ -16,33 +16,34 @@
-include("couch_server_int.hrl").
new() ->
- {gb_trees:empty(), dict:new()}.
+ % {gb_trees(UniqueInt -> DbName), #{DbName => UniqueInt}}
+ {gb_trees:empty(), #{}}.
-insert(DbName, {Tree0, Dict0}) ->
+insert(DbName, {Tree0, #{} = Dict0}) ->
Review Comment:
Maybe rename `Dict` vars to `Map`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "jaydoane (via GitHub)" <gi...@apache.org>.
jaydoane commented on code in PR #5165:
URL: https://github.com/apache/couchdb/pull/5165#discussion_r1702939292
##########
src/couch/src/couch_lru.erl:
##########
@@ -16,33 +16,34 @@
-include("couch_server_int.hrl").
new() ->
- {gb_trees:empty(), dict:new()}.
+ % {gb_trees(UniqueInt -> DbName), #{DbName => UniqueInt}}
+ {gb_trees:empty(), #{}}.
-insert(DbName, {Tree0, Dict0}) ->
+insert(DbName, {Tree0, #{} = Dict0}) ->
Review Comment:
I wonder if some `-spec` statements on the public functions might also be helpful :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix and improve couch_lru [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5165:
URL: https://github.com/apache/couchdb/pull/5165#discussion_r1702393733
##########
src/couch/src/couch_lru.erl:
##########
@@ -53,14 +54,19 @@ close_int({Lru, DbName, Iter}, {Tree, Dict} = Cache) ->
true = ets:delete(CouchDbs, DbName),
true = ets:delete(CouchDbsPidToName, Pid),
exit(Pid, kill),
- {true, {gb_trees:delete(Lru, Tree), dict:erase(DbName, Dict)}};
+ {true, {gb_trees:delete(Lru, Tree), maps:remove(DbName, Dict)}};
false ->
true = couch_server:unlock(CouchDbs, DbName),
couch_stats:increment_counter([couchdb, couch_server, lru_skip]),
- close_int(gb_trees:next(Iter), update(DbName, Cache))
+ NewCache = {NewTree, _} = update(DbName, Cache),
+ close_int(larger(Lru, NewTree), NewCache)
Review Comment:
This fixes trees getting out of sync, but gets us in another trouble: non-termination. If all the entries are not idle, then we'd be bumping them up as we see them. The `Lru` key would increase, for example, from 5 to 11, then we see the key again 11, and we bump it again, now it's at say 13, we bump it, and so on...
We'll have to figure something else out here. We've been discussing batching the entries needing an update for after close_int returns perhaps....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org