[Asterisk-code-review] named locks: Use ao2 weakproxy to deal with cleanup from con... (asterisk[master])

[Corey Farrell]

Corey Farrell has posted comments on this change.

Change subject: named_locks: Use ao2_weakproxy to deal with cleanup from container.
......................................................................


Patch Set 1:

(1 comment)

> (1 comment)
 > 
 > How much do you think we save on the locking vs the addition of the
 > extra object and management?

It's hard to say, the usage pattern is a big variable in this question.  Without the next patch (3748) this change is bad.  With the next patch I suspect this change will be an improvement, but I don't have profiling data to support this.  Further I don't know if profiling data can be gotten/helpful since the improvement is reduced contention for the named_locks global lock.

Really the old usage pattern isn't very good even without this change.  From what I can tell the normal execution path for ast_named_lock_get would be a NULL return from ao2_find, followed by the allocation / ao2_link.  Including the next change makes it so ast_named_lock_get(AST_NAMED_LOCK_TYPE_MUTEX, "aor", "alice") will result in ao2_alloc the first time, then reuse the existing object on following calls.  This all assumes that sorcery reload causes the new aor/alice object to be created before the old is destroyed.  So yes in the new change we are keeping more memory, but depending on how many reloads are done we might be doing less object management.

https://gerrit.asterisk.org/#/c/3747/1/include/asterisk/named_locks.h
File include/asterisk/named_locks.h:

Line 93: #define ast_named_lock_put(lock) ao2_cleanup(lock)
> My concern here is if we wanted to backport this patch, it'd break ABI.  Ma
For a backport to 14 we would keep the ABI and wrap ao2_cleanup, no point doing that to master.  Since this change requires ao2_weakproxy I was not considering it for 13.


-- 
To view, visit https://gerrit.asterisk.org/3747
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I644e39c6d83a153d71b3fae77ec05599d725e7e6
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-HasComments: Yes