You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Igor Galić <i....@brainsware.org> on 2013/02/01 19:36:27 UTC
Re: git commit: TS-1678: Simplify register_record()
On Friday, February 01, 2013 04:36:37 PM jpeach@apache.org wrote:
> Updated Branches:
> refs/heads/master baca4d2b7 -> 5f93bc50f
>
>
> TS-1678: Simplify register_record()
>
> 'release_record_lock' parameter to register_record() is unnecessarily
> complicated and the lock release is dangerous. Let's simplify it
I'm all for removing code but would you care to explain why "the lock release
is dangerous" ?
-- i
> and remove verbose lock releasing from the upper layer code.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/5f93bc50
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/5f93bc50
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/5f93bc50
>
> Branch: refs/heads/master
> Commit: 5f93bc50fd1869f4eb2866ecffd92fc616835fb4
> Parents: baca4d2
> Author: Yunkai Zhang <qi...@taobao.com>
> Authored: Fri Feb 1 08:35:07 2013 -0800
> Committer: James Peach <jp...@apache.org>
> Committed: Fri Feb 1 08:35:07 2013 -0800
>
> ----------------------------------------------------------------------
> CHANGES | 3 +++
> lib/records/RecCore.cc | 18 +++---------------
> 2 files changed, 6 insertions(+), 15 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/5f93bc50/CHANGES
> ----------------------------------------------------------------------
> diff --git a/CHANGES b/CHANGES
> index ad32f87..9da2ed4 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -1,6 +1,9 @@
> -*- coding: utf-8
> -*- Changes with Apache Traffic Server 3.3.1
>
> + *) [TS-1678] Simplify register_record
> + Author: Yunkai Zhang <qi...@taobao.com>
> +
> *) [TS-1252] stats summary in cluster not working
> Author: Yunkai Zhang <qi...@taobao.com>
>
>
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/5f93bc50/lib/recor
> ds/RecCore.cc
> ---------------------------------------------------------------------- diff
> --git a/lib/records/RecCore.cc b/lib/records/RecCore.cc
> index d9215ed..4888fbb 100644
> --- a/lib/records/RecCore.cc
> +++ b/lib/records/RecCore.cc
> @@ -52,14 +52,11 @@ RecTree *g_records_tree = NULL;
> // register_record
> //-------------------------------------------------------------------------
> static RecRecord *
> -register_record(RecT rec_type, const char *name, RecDataT data_type,
> RecData data_default, bool release_record_lock) +register_record(RecT
> rec_type, const char *name, RecDataT data_type, RecData data_default) {
> RecRecord *r = NULL;
>
> if (ink_hash_table_lookup(g_records_ht, name, (void **) &r)) {
> - if (release_record_lock) {
> - rec_mutex_acquire(&(r->lock));
> - }
> ink_release_assert(r->rec_type == rec_type);
> ink_release_assert(r->data_type == data_type);
> // Note: do not set r->data as we want to keep the previous value
> @@ -68,9 +65,6 @@ register_record(RecT rec_type, const char *name, RecDataT
> data_type, RecData dat if ((r = RecAlloc(rec_type, name, data_type)) ==
> NULL) {
> return NULL;
> }
> - if (release_record_lock) {
> - rec_mutex_acquire(&(r->lock));
> - }
> // Set the r->data to its default value as this is a new record
> RecDataSet(r->data_type, &(r->data), &(data_default));
> RecDataSet(r->data_type, &(r->data_default), &(data_default));
> @@ -80,10 +74,6 @@ register_record(RecT rec_type, const char *name, RecDataT
> data_type, RecData dat // we're now registered
> r->registered = true;
>
> - if (release_record_lock) {
> - rec_mutex_release(&(r->lock));
> - }
> -
> return r;
> }
>
> @@ -733,9 +723,8 @@ RecRegisterStat(RecT rec_type, const char *name,
> RecDataT data_type, RecData dat RecRecord *r = NULL;
>
> ink_rwlock_wrlock(&g_records_rwlock);
> - if ((r = register_record(rec_type, name, data_type, data_default, false))
> != NULL) { + if ((r = register_record(rec_type, name, data_type,
> data_default)) != NULL) { r->stat_meta.persist_type = persist_type;
> - rec_mutex_release(&(r->lock));
> } else {
> ink_debug_assert(!"Can't register record!");
> }
> @@ -755,7 +744,7 @@ RecRegisterConfig(RecT rec_type, const char *name,
> RecDataT data_type, {
> RecRecord *r;
> ink_rwlock_wrlock(&g_records_rwlock);
> - if ((r = register_record(rec_type, name, data_type, data_default, false))
> != NULL) { + if ((r = register_record(rec_type, name, data_type,
> data_default)) != NULL) { // Note: do not modify
> 'record->config_meta.update_required'
> r->config_meta.update_type = update_type;
> r->config_meta.check_type = check_type;
> @@ -765,7 +754,6 @@ RecRegisterConfig(RecT rec_type, const char *name,
> RecDataT data_type, r->config_meta.check_expr = ats_strdup(check_expr);
> r->config_meta.update_cb_list = NULL;
> r->config_meta.access_type = access_type;
> - rec_mutex_release(&(r->lock));
> }
> ink_rwlock_unlock(&g_records_rwlock);
Re: git commit: TS-1678: Simplify register_record()
Posted by Yunkai Zhang <yu...@gmail.com>.
在 2013年2月2日星期六，Igor Galić 写道：
> On Friday, February 01, 2013 04:36:37 PM jpeach@apache.org <javascript:;>wrote:
> > Updated Branches:
> > refs/heads/master baca4d2b7 -> 5f93bc50f
> >
> >
> > TS-1678: Simplify register_record()
> >
> > 'release_record_lock' parameter to register_record() is unnecessarily
> > complicated and the lock release is dangerous. Let's simplify it
>
> I'm all for removing code but would you care to explain why "the lock
> release
> is dangerous" ?
>
>
Releasing a lock before acquiring it is a bad behavior.
Although there is a reference count, but if the count > 0, the releasing
operation will decrease it unsafely.
> -- i
>
--
Yunkai Zhang
Work at Taobao