On 25-Jun-15, Bram Moolenaar wrote:
>
> Olaf Dabrunz wrote:
>
> > This needed more time and work on more realistic tests, and to create a
> > fix that addresses all issues. Sorry for the delay.
> >
> > Summary:
> >
> > - Bug: locks can result in false error reports or missing error
> > reports when the locks are not interpreted in the same way by all
> > vim functions and commands.
> >
> > When this is fixed, locks
> >
> > - can reliably be used to find mistakes
> >
> > - do not terminate production runs (or test runs) because of
> > unexpected false positives.
> >
> > - Bug: on an attempt to change a locked item, extend() is left with
> > an error and all previous changes are kept (exit-on-error).
> >
> > - Bug: when the target dict is locked, extend() reports false errors
> > for 'extend(d, {})' and 'extend({"a": 1}, {"a": 2}, "keep")'.
> >
> > - New, less artificial tests show that a lock check loop adds less
> > overhead than suggested by previous tests, esp. for common use
> > cases.
> >
> > For the more common use cases, a separate lock test loop adds an
> > overhead of
> >
> > avg.: 17% stddev: ± 16, max. 40%.
> >
> > Across the whole range of use cases, the tests show an overhead of
> >
> > avg.: 34% stddev: ± 27, max. 100%.
> >
> > These tests are still somewhat artificial. The overhead shrinks
> > beyond these values when larger item values (e.g. real-life
> > strings) are used.
> >
> > - Fix: all above bugs are fixed for extend() by using a lock check
> > loop that is only used when the target dict has locks set.
> >
> > This implements all-or-nothing behavior at near-zero cost when no
> > locks are used on the target dict.
> >
> > When locks are used on the target dict, the slowdown is as shown
> > in above tests.
> >
> > - Open issue: are lock check loops needed in other situations (e.g.
> > for lists), and should a similar lock count optimization be used?
>
> This is a long message...
My apologies.
I do not like long mails. They take too much time to read, and to
answer. Not good for a discussion.
After several attempts to make it shorter, this is the best I could do
while addressing all concerns. Still could improve... (I hope the
summary saves my karma...)
> This is a long message... Your reasoning makes sense.
Thanks! :)
> This is a long message... Your reasoning makes sense. Anybody else has
> a problem with the proposed solution?
>
> dv_ilocked is not an obvious name, perhaps dv_has_locks?
Much better! :) It's in the patch below.
(Seriously, why could I not come up with this...)
I could have gleaned it from this comment:
/* When d1 has locks, first check for lock breaks and overwrite errors. */
if (d1->dv_has_locks || d1->dv_lock & (VAR_LOCKED | VAR_FIXED))
... but I didn't... sigh...
> The loop to check for locks is very similar to the loop doing the work.
> Consider using the same loop and run it twice, once to check and once to
> do the work. Avoids that later only one of the two loops is modified.
Yes.
I tried before, but did not want to put so many 'if's into the loop, so
that the other error checks would not be done twice.
But 'if's do not really matter much, with branch prediction. Also,
redoing the checks should not matter much, as cache fill is the bottle
neck. It could still matter for small dicts though, as they may remain
in the cache.
So I put in some 'if's, so that lock checks and variable name checks
would not be done twice. The change "error" checks are repeated though,
which is the most simple way to make sure these are reported in both
the cases with or without locks. They are also the cheapest checks.
NOTE: Please use the patch below in this message. The attached patch is
for review only, it is much more readable when whitespace is ignored.
---
src/eval.c | 106 ++++++++++++++++++++++++++++++++------------------
src/structs.h | 1
src/testdir/test55.in | 24 +++++++++++
src/testdir/test55.ok | 7 +++
4 files changed, 101 insertions(+), 37 deletions(-)
Index: b/src/eval.c
===================================================================
--- a/src/eval.c 2015-06-21 15:43:04.278647940 +0200
+++ b/src/eval.c 2015-06-25 13:45:17.753217769 +0200
@@ -3810,8 +3810,11 @@ do_lock_var(lp, name_end, deep, lock)
/* (un)lock a List item. */
item_lock(&lp->ll_li->li_tv, deep, lock);
else
+ {
/* un(lock) a Dictionary item. */
item_lock(&lp->ll_di->di_tv, deep, lock);
+ lp->ll_dict->dv_has_locks += lock ? 1 : -1;
+ }
return ret;
}
@@ -3879,6 +3882,7 @@ item_lock(tv, deep, lock)
{
--todo;
item_lock(&HI2DI(hi)->di_tv, deep - 1, lock);
+ d->dv_has_locks += lock ? 1 : -1;
}
}
}
@@ -7195,6 +7199,7 @@ dict_alloc()
hash_init(&d->dv_hashtab);
d->dv_lock = 0;
+ d->dv_has_locks = 0;
d->dv_scope = 0;
d->dv_refcount = 0;
d->dv_copyID = 0;
@@ -10507,46 +10512,73 @@ dict_extend(d1, d2, action)
dictitem_T *di1;
hashitem_T *hi2;
int todo;
+ int check_locks = 0;
char_u *arg_errmsg = (char_u *)N_("extend() argument");
- todo = (int)d2->dv_hashtab.ht_used;
- for (hi2 = d2->dv_hashtab.ht_array; todo > 0; ++hi2)
+ /* When d1 has locks, first check for item change errors and lock breaks.
*/
+ if (d1->dv_has_locks || d1->dv_lock & (VAR_LOCKED | VAR_FIXED))
+ check_locks = 1;
+
+ /*
+ * check_locks:
+ * 1: Check for item change errors and lock breaks, make NO changes.
+ * 0: Check for errors and make changes.
+ */
+ for ( ; check_locks >= 0; --check_locks)
{
- if (!HASHITEM_EMPTY(hi2))
+ todo = (int)d2->dv_hashtab.ht_used;
+ for (hi2 = d2->dv_hashtab.ht_array; todo > 0; ++hi2)
{
- --todo;
- di1 = dict_find(d1, hi2->hi_key, -1);
- if (d1->dv_scope != 0)
- {
- /* Disallow replacing a builtin function in l: and g:.
- * Check the key to be valid when adding to any
- * scope. */
- if (d1->dv_scope == VAR_DEF_SCOPE
- && HI2DI(hi2)->di_tv.v_type == VAR_FUNC
- && var_check_func_name(hi2->hi_key,
- di1 == NULL))
- break;
- if (!valid_varname(hi2->hi_key))
- break;
- }
- if (di1 == NULL)
+ if (!HASHITEM_EMPTY(hi2))
{
- di1 = dictitem_copy(HI2DI(hi2));
- if (di1 != NULL && dict_add(d1, di1) == FAIL)
- dictitem_free(di1);
- }
- else if (*action == 'e')
- {
- EMSG2(_("E737: Key already exists: %s"), hi2->hi_key);
- break;
- }
- else if (*action == 'f' && HI2DI(hi2) != di1)
- {
- if (tv_check_lock(di1->di_tv.v_lock, arg_errmsg, TRUE)
- || var_check_ro(di1->di_flags, arg_errmsg, TRUE))
- break;
- clear_tv(&di1->di_tv);
- copy_tv(&HI2DI(hi2)->di_tv, &di1->di_tv);
+ --todo;
+ di1 = dict_find(d1, hi2->hi_key, -1);
+ if (!check_locks && d1->dv_scope != 0)
+ {
+ /* Disallow replacing a builtin function in l: and g:.
+ * Check the key to be valid when adding to any
+ * scope. */
+ if (d1->dv_scope == VAR_DEF_SCOPE
+ && HI2DI(hi2)->di_tv.v_type == VAR_FUNC
+ && var_check_func_name(hi2->hi_key,
+ di1 == NULL))
+ return;
+ if (!valid_varname(hi2->hi_key))
+ return;
+ }
+ if (di1 == NULL)
+ {
+ if (check_locks)
+ {
+ if (tv_check_lock(d1->dv_lock, arg_errmsg, TRUE))
+ return;
+ }
+ else
+ {
+ di1 = dictitem_copy(HI2DI(hi2));
+ if (di1 != NULL && dict_add(d1, di1) == FAIL)
+ dictitem_free(di1);
+ }
+ }
+ else if (*action == 'e')
+ {
+ EMSG2(_("E737: Key already exists: %s"), hi2->hi_key);
+ return;
+ }
+ else if (*action == 'f' && HI2DI(hi2) != di1)
+ {
+ if (check_locks)
+ {
+ if (tv_check_lock(di1->di_tv.v_lock, arg_errmsg, TRUE)
+ || var_check_ro(di1->di_flags, arg_errmsg, TRUE))
+ return;
+ }
+ else
+ {
+ clear_tv(&di1->di_tv);
+ copy_tv(&HI2DI(hi2)->di_tv, &di1->di_tv);
+ }
+ }
}
}
}
@@ -10608,8 +10640,7 @@ f_extend(argvars, rettv)
d1 = argvars[0].vval.v_dict;
d2 = argvars[1].vval.v_dict;
- if (d1 != NULL && !tv_check_lock(d1->dv_lock, arg_errmsg, TRUE)
- && d2 != NULL)
+ if (d1 != NULL && d2 != NULL)
{
/* Check the third argument. */
if (argvars[2].v_type != VAR_UNKNOWN)
@@ -21330,6 +21361,7 @@ init_var_dict(dict, dict_var, scope)
{
hash_init(&dict->dv_hashtab);
dict->dv_lock = 0;
+ dict->dv_has_locks = 1; /* must expect DI_FLAGS_RO* on items */
dict->dv_scope = scope;
dict->dv_refcount = DO_NOT_FREE_CNT;
dict->dv_copyID = 0;
Index: b/src/structs.h
===================================================================
--- a/src/structs.h 2015-06-21 15:42:20.922696781 +0200
+++ b/src/structs.h 2015-06-25 13:14:13.959954566 +0200
@@ -1219,6 +1219,7 @@ struct dictvar_S
int dv_refcount; /* reference count */
int dv_copyID; /* ID used by deepcopy() */
hashtab_T dv_hashtab; /* hashtab that refers to the items */
+ long_u dv_has_locks; /* locked items + 1 if DI_FLAGS_RO* possible */
dict_T *dv_copydict; /* copied dict used by deepcopy() */
dict_T *dv_used_next; /* next dict in used dicts list */
dict_T *dv_used_prev; /* previous dict in used dicts list */
Index: b/src/testdir/test55.in
===================================================================
--- a/src/testdir/test55.in 2015-06-21 15:42:20.922696781 +0200
+++ b/src/testdir/test55.in 2015-06-21 20:26:18.759475460 +0200
@@ -408,6 +408,30 @@ let l = [0, 1, 2, 3]
:endtry
:$put =string(d)
:"
+:$put ='extend() after lock on dict:'
+:unlet! d
+:let d = {'a': 99, 'b': 100, 'd': 101}
+:lockvar 1 d
+:$put =string(extend(d, {'a': 123}))
+:try
+: $put =string(extend(d, {'x': 'new'}))
+: $put ='wrong: did extend() with new item'
+:catch
+: $put =v:exception[:14]
+:endtry
+:$put =string(d)
+:unlet! d2
+:let d2 = {'a': 789, 'b': 42, 'c': 'new', 'd': 52}
+:$put ='key order is ok to recognize error modes: '.(keys(d2)[0] !=
'c').(keys(d2)[-1] != 'c')
+:try
+: $put =string(extend(d, d2))
+: $put ='wrong: did extend() with mixed-in new item'
+:catch
+: $put =v:exception[:14]
+:endtry
+:$put =string(d)
+:unlet d d2
+:"
:$put ='No remove() of write-protected scope-level variable:'
:fun! Tfunc(this_is_a_loooooooooong_parameter_name)
: try
Index: b/src/testdir/test55.ok
===================================================================
--- a/src/testdir/test55.ok 2015-06-21 15:42:20.922696781 +0200
+++ b/src/testdir/test55.ok 2015-06-21 20:26:18.763475458 +0200
@@ -138,6 +138,13 @@ did map()
No extend() after lock on dict item:
Vim(put):E741:
{'a': 99, 'b': 100}
+extend() after lock on dict:
+{'a': 123, 'b': 100, 'd': 101}
+Vim(put):E741:
+{'a': 123, 'b': 100, 'd': 101}
+key order is ok to recognize error modes: 11
+Vim(put):E741:
+{'a': 123, 'b': 100, 'd': 101}
No remove() of write-protected scope-level variable:
Vim(put):E795:
No extend() of write-protected scope-level variable:
--
Olaf Dabrunz (oda <at> fctrace.org)
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
---
src/eval.c | 46 +++++++++++++++++++++++++++++++++++++++-------
src/structs.h | 1 +
src/testdir/test55.in | 24 ++++++++++++++++++++++++
src/testdir/test55.ok | 7 +++++++
4 files changed, 71 insertions(+), 7 deletions(-)
Index: b/IGNORESPACE/src/eval.c
===================================================================
--- a/IGNORESPACE/src/eval.c 2015-06-21 15:43:04.278647940 +0200
+++ b/IGNORESPACE/src/eval.c 2015-06-25 13:45:17.753217769 +0200
@@ -3810,8 +3810,11 @@ do_lock_var(lp, name_end, deep, lock)
/* (un)lock a List item. */
item_lock(&lp->ll_li->li_tv, deep, lock);
else
+ {
/* un(lock) a Dictionary item. */
item_lock(&lp->ll_di->di_tv, deep, lock);
+ lp->ll_dict->dv_has_locks += lock ? 1 : -1;
+ }
return ret;
}
@@ -3879,6 +3882,7 @@ item_lock(tv, deep, lock)
{
--todo;
item_lock(&HI2DI(hi)->di_tv, deep - 1, lock);
+ d->dv_has_locks += lock ? 1 : -1;
}
}
}
@@ -7195,6 +7199,7 @@ dict_alloc()
hash_init(&d->dv_hashtab);
d->dv_lock = 0;
+ d->dv_has_locks = 0;
d->dv_scope = 0;
d->dv_refcount = 0;
d->dv_copyID = 0;
@@ -10507,8 +10512,20 @@ dict_extend(d1, d2, action)
dictitem_T *di1;
hashitem_T *hi2;
int todo;
+ int check_locks = 0;
char_u *arg_errmsg = (char_u *)N_("extend() argument");
+ /* When d1 has locks, first check for item change errors and lock breaks. */
+ if (d1->dv_has_locks || d1->dv_lock & (VAR_LOCKED | VAR_FIXED))
+ check_locks = 1;
+
+ /*
+ * check_locks:
+ * 1: Check for item change errors and lock breaks, make NO changes.
+ * 0: Check for errors and make changes.
+ */
+ for ( ; check_locks >= 0; --check_locks)
+ {
todo = (int)d2->dv_hashtab.ht_used;
for (hi2 = d2->dv_hashtab.ht_array; todo > 0; ++hi2)
{
@@ -10516,7 +10533,7 @@ dict_extend(d1, d2, action)
{
--todo;
di1 = dict_find(d1, hi2->hi_key, -1);
- if (d1->dv_scope != 0)
+ if (!check_locks && d1->dv_scope != 0)
{
/* Disallow replacing a builtin function in l: and g:.
* Check the key to be valid when adding to any
@@ -10525,32 +10542,47 @@ dict_extend(d1, d2, action)
&& HI2DI(hi2)->di_tv.v_type == VAR_FUNC
&& var_check_func_name(hi2->hi_key,
di1 == NULL))
- break;
+ return;
if (!valid_varname(hi2->hi_key))
- break;
+ return;
}
if (di1 == NULL)
{
+ if (check_locks)
+ {
+ if (tv_check_lock(d1->dv_lock, arg_errmsg, TRUE))
+ return;
+ }
+ else
+ {
di1 = dictitem_copy(HI2DI(hi2));
if (di1 != NULL && dict_add(d1, di1) == FAIL)
dictitem_free(di1);
}
+ }
else if (*action == 'e')
{
EMSG2(_("E737: Key already exists: %s"), hi2->hi_key);
- break;
+ return;
}
else if (*action == 'f' && HI2DI(hi2) != di1)
{
+ if (check_locks)
+ {
if (tv_check_lock(di1->di_tv.v_lock, arg_errmsg, TRUE)
|| var_check_ro(di1->di_flags, arg_errmsg, TRUE))
- break;
+ return;
+ }
+ else
+ {
clear_tv(&di1->di_tv);
copy_tv(&HI2DI(hi2)->di_tv, &di1->di_tv);
}
}
}
}
+ }
+}
/*
* "extend(list, list [, idx])" function
@@ -10608,8 +10640,7 @@ f_extend(argvars, rettv)
d1 = argvars[0].vval.v_dict;
d2 = argvars[1].vval.v_dict;
- if (d1 != NULL && !tv_check_lock(d1->dv_lock, arg_errmsg, TRUE)
- && d2 != NULL)
+ if (d1 != NULL && d2 != NULL)
{
/* Check the third argument. */
if (argvars[2].v_type != VAR_UNKNOWN)
@@ -21330,6 +21361,7 @@ init_var_dict(dict, dict_var, scope)
{
hash_init(&dict->dv_hashtab);
dict->dv_lock = 0;
+ dict->dv_has_locks = 1; /* must expect DI_FLAGS_RO* on items */
dict->dv_scope = scope;
dict->dv_refcount = DO_NOT_FREE_CNT;
dict->dv_copyID = 0;
Index: b/IGNORESPACE/src/structs.h
===================================================================
--- a/IGNORESPACE/src/structs.h 2015-06-21 15:42:20.922696781 +0200
+++ b/IGNORESPACE/src/structs.h 2015-06-25 13:14:13.959954566 +0200
@@ -1219,6 +1219,7 @@ struct dictvar_S
int dv_refcount; /* reference count */
int dv_copyID; /* ID used by deepcopy() */
hashtab_T dv_hashtab; /* hashtab that refers to the items */
+ long_u dv_has_locks; /* locked items + 1 if DI_FLAGS_RO* possible */
dict_T *dv_copydict; /* copied dict used by deepcopy() */
dict_T *dv_used_next; /* next dict in used dicts list */
dict_T *dv_used_prev; /* previous dict in used dicts list */
Index: b/IGNORESPACE/src/testdir/test55.in
===================================================================
--- a/IGNORESPACE/src/testdir/test55.in 2015-06-21 15:42:20.922696781 +0200
+++ b/IGNORESPACE/src/testdir/test55.in 2015-06-21 20:26:18.759475460 +0200
@@ -408,6 +408,30 @@ let l = [0, 1, 2, 3]
:endtry
:$put =string(d)
:"
+:$put ='extend() after lock on dict:'
+:unlet! d
+:let d = {'a': 99, 'b': 100, 'd': 101}
+:lockvar 1 d
+:$put =string(extend(d, {'a': 123}))
+:try
+: $put =string(extend(d, {'x': 'new'}))
+: $put ='wrong: did extend() with new item'
+:catch
+: $put =v:exception[:14]
+:endtry
+:$put =string(d)
+:unlet! d2
+:let d2 = {'a': 789, 'b': 42, 'c': 'new', 'd': 52}
+:$put ='key order is ok to recognize error modes: '.(keys(d2)[0] != 'c').(keys(d2)[-1] != 'c')
+:try
+: $put =string(extend(d, d2))
+: $put ='wrong: did extend() with mixed-in new item'
+:catch
+: $put =v:exception[:14]
+:endtry
+:$put =string(d)
+:unlet d d2
+:"
:$put ='No remove() of write-protected scope-level variable:'
:fun! Tfunc(this_is_a_loooooooooong_parameter_name)
: try
Index: b/IGNORESPACE/src/testdir/test55.ok
===================================================================
--- a/IGNORESPACE/src/testdir/test55.ok 2015-06-21 15:42:20.922696781 +0200
+++ b/IGNORESPACE/src/testdir/test55.ok 2015-06-21 20:26:18.763475458 +0200
@@ -138,6 +138,13 @@ did map()
No extend() after lock on dict item:
Vim(put):E741:
{'a': 99, 'b': 100}
+extend() after lock on dict:
+{'a': 123, 'b': 100, 'd': 101}
+Vim(put):E741:
+{'a': 123, 'b': 100, 'd': 101}
+key order is ok to recognize error modes: 11
+Vim(put):E741:
+{'a': 123, 'b': 100, 'd': 101}
No remove() of write-protected scope-level variable:
Vim(put):E795:
No extend() of write-protected scope-level variable:
