On 30-Apr-15, Bram Moolenaar wrote:
>
> Olaf Dabrunz wrote:
>
> > Example:
> >
> > :let d = {'a': 99, 'b': 100}
> > :lockvar 1 d
> > :call extend(d, {'a': 123})
> > E741: Value is locked: extend() argument
> > :let d.a = 123
> > :let d
> > d {'a': 123, 'b': 100}
> >
> > The dictionary is locked, but the 'a' item is not, so extend() on 'a'
> > should succeed in the same way as the 'let' does. According to ':he
> > lockvar', a dictionary lock protects against item deletion and addition,
> > but not against changing an existing item.
>
> Hmm, one could argue that extend() adds the new key/value pair, thus
> that it should fail.
For lists, I agree -- extend(list, list2[, where]) does not touch
existing items.
For dicts, extend() modifies existing items and adds new items when
their keys do not exist yet.
> Zyx's argument that it may change one existing key/value but then fail
> on a following one that adds a key makes sense.
This was also discussed by you and ZyX for patch 7.4.419 ("when part of
a list is locked it's possible to make changes"), which is a related
situation. ZyX proposed several approaches, and the result was "[list]
items will be removed [or changed] only if all items are not locked".
> Perhaps we should just
> state "extend() on a locked list or dict does not work".
If we let extend() fail here, then ':let' (and map()) should fail as
well. The lock should not have a different effect when the change is
attempted by different means.
This would be a different lock model.
The all-or-nothing semantics you and ZyX mention can be implemented for
extend(dict, dict2[, how]) as well. But note:
- extend(dict, dict2[, how]) currently already has several
exit-on-error conditions:
- adding a function to scopes l: or g: that conflicts with a
builtin function
- adding or replacing an item in a variable scope, when the key
is not a valid variable name
- changing an existing dict item while 'how' is 'error'
- changing a locked dict item (since 7.4.698).
For all-or-nothing semantics, all of these need to be moved into a
separate checking loop.
(This is the reason why I considered all-or-nothing semantics
beyond the scope of my patch: the current implementation has
exit-on-error semantics.)
- A separate checking loop needs to repeat all dictionary lookups.
For my tests, this made extend(dict, ...) take twice as much time
to complete.
:let d = {}
:for i in range(10000) | let d[i] = 't'.i | endfor
:for t in range(3)
: let start = reltime()
: for i in range(10000) | call extend(d, d) | endfor
: echomsg reltimestr(reltime(start))
:endfor
Unpatched:
3.255444
3.243517
3.248523
Old patch (exit-on-error semantics):
3.249932
3.256253
3.255438
New patch with separate checking loop (see below):
6.452288
6.390474
6.420924
I am not sure if taking twice as long is a good tradeoff for
having all-or-nothing semantics.
- In case of an out of memory error, extend() skips that item and
continues with the next one (continue-after-error semantics).
For OOM, all-or-nothing semantics can only be achieved by
implementing a rollback, because OOM can only happen when we
actually execute the changes.
A rollback requires copies of the changed items. Making them is
relatively expensive.
Note that list functions do not implement this: 'extend(list,
list2, how)' and ':let list[i:j] = list2' and probably others do
have exit-on-OOM semantics.
One reason for not implementing all-or-nothing for OOM may be that
OOM rarely happens: as discussed in another thread, users tend to
terminate other processes before OOM (to avoid a slow system that
is swapping or thrashing), or an OOM-killer may kill some other
process to avoid OOM.
It may not be worth the slowdown just to have a rollback on OOM.
An updated patch that implements all-or-nothing semantics in
extend(dict, dict2[, how]) (except for OOM) is at the end of this mail.
On another note, while looking at the 7.4.419 patch again, I realized
that it adds checks to ':unlet' for item locks before it deletes any
items. This is not needed under the locking model described in ':he
lockvar': item locks protect against item change, not against item
deletion.
The list lock, which protects against deletion, is checked before
already.
Here is a patch that removes the unneeded item lock checks. Note that
no changes to the tests are needed, as the list lock already makes sure
the items cannot be deleted.
diff -r 18d84ed365a5 src/eval.c
--- a/src/eval.c Wed Apr 22 22:18:22 2015 +0200
+++ b/src/eval.c Fri May 01 09:50:23 2015 +0200
@@ -3669,15 +3676,6 @@ do_unlet_var(lp, name_end, forceit)
listitem_T *ll_li = lp->ll_li;
int ll_n1 = lp->ll_n1;
- while (ll_li != NULL && (lp->ll_empty2 || lp->ll_n2 >= ll_n1))
- {
- li = ll_li->li_next;
- if (tv_check_lock(ll_li->li_tv.v_lock, lp->ll_name, FALSE))
- return FAIL;
- ll_li = li;
- ++ll_n1;
- }
-
/* Delete a range of List items. */
while (lp->ll_li != NULL && (lp->ll_empty2 || lp->ll_n2 >= lp->ll_n1))
{
Here is the updated extend() patch with all-or-nothing semantics:
diff -r 18d84ed365a5 src/eval.c
--- a/src/eval.c Wed Apr 22 22:18:22 2015 +0200
+++ b/src/eval.c Sat May 02 02:57:53 2015 +0200
@@ -10502,6 +10502,10 @@ dict_extend(d1, d2, action)
int todo;
char_u *arg_errmsg = (char_u *)N_("extend() argument");
+ /*
+ * Check whether the operation can complete without errors (other than
+ * OOM).
+ */
todo = (int)d2->dv_hashtab.ht_used;
for (hi2 = d2->dv_hashtab.ht_array; todo > 0; ++hi2)
{
@@ -10518,26 +10522,45 @@ 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 (tv_check_lock(d1->dv_lock, arg_errmsg, TRUE))
+ return;
+ }
+ else if (*action == 'e')
+ {
+ EMSG2(_("E737: Key already exists: %s"), hi2->hi_key);
+ return;
+ }
+ else if (*action == 'f' && HI2DI(hi2) != di1
+ && (tv_check_lock(di1->di_tv.v_lock, arg_errmsg, TRUE)
+ || var_check_ro(di1->di_flags, arg_errmsg, TRUE)))
+ return;
+ }
+ }
+
+ /*
+ * Replace or add dict items.
+ */
+ todo = (int)d2->dv_hashtab.ht_used;
+ for (hi2 = d2->dv_hashtab.ht_array; todo > 0; ++hi2)
+ {
+ if (!HASHITEM_EMPTY(hi2))
+ {
+ --todo;
+ di1 = dict_find(d1, hi2->hi_key, -1);
if (di1 == NULL)
{
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);
}
@@ -10601,8 +10624,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)
diff -r 18d84ed365a5 src/testdir/test55.in
--- a/src/testdir/test55.in Wed Apr 22 22:18:22 2015 +0200
+++ b/src/testdir/test55.in Sat May 02 02:57:53 2015 +0200
@@ -408,6 +408,27 @@ let l = [0, 1, 2, 3]
:endtry
:$put =string(d)
:"
+:$put ='extend() after lock on dict:'
+:unlet! d
+:let d = {'a': 99, 'b': 100, 'c': 101}
+:lockvar 1 d
+:try
+: $put =string(extend(d, {'a': 123}))
+: $put ='ok: did extend() existing item'
+: $put =string(extend(d, {'x': 'new'}))
+: $put ='wrong: did extend() with new item'
+:catch
+: $put =v:exception[:14]
+:endtry
+:$put =string(d)
+:try
+: $put =string(extend(d, {'a': 789, 'x': 'new', 'b': 42, 'c': 52}))
+:catch
+: $put =v:exception[:14]
+:endtry
+:$put =string(d)
+:unlet d
+:"
:$put ='No remove() of write-protected scope-level variable:'
:fun! Tfunc(this_is_a_loooooooooong_parameter_name)
: try
diff -r 18d84ed365a5 src/testdir/test55.ok
--- a/src/testdir/test55.ok Wed Apr 22 22:18:22 2015 +0200
+++ b/src/testdir/test55.ok Sat May 02 02:57:53 2015 +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, 'c': 101}
+ok: did extend() existing item
+Vim(put):E741:
+{'a': 123, 'b': 100, 'c': 101}
+Vim(put):E741:
+{'a': 123, 'b': 100, 'c': 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.
