[issue19385] dbm.dumb should be consistent when the database is closed

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Thanks Claudiu.

--
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Roundup Robot]

Roundup Robot added the comment:

New changeset 95207bcd8298 by Serhiy Storchaka in branch '3.4':
Restore performance of some dumb database methods (regression introduced by 
#19385).
http://hg.python.org/cpython/rev/95207bcd8298

New changeset 2e59e0b579e5 by Serhiy Storchaka in branch 'default':
Restore performance of some dumb database methods (regression introduced by 
#19385).
http://hg.python.org/cpython/rev/2e59e0b579e5

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

Looks good to me.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

There is no need to speed up methods which do IO (__getitem__, __setitem__, 
__delitem__). However method which works only with an index (keys, iterkeys, 
__contains__, __len__) can be optimized. In the __contains__ method an 
exception can be raised not only by nen-existent __contains__ of None, but from 
__hash__ or __eq__ methods of a key, so we should distinguish these cases.

--
stage: resolved -> patch review
versions: +Python 3.4
Added file: http://bugs.python.org/file35348/issue19385_speed_2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

Here's a new patch which uses the EAFP approach for dunder methods (__len__, 
__contains__ etc) and the _verify_open method for the other methods (.keys, 
.iterkeys) etc. Now the results are similar with the benchmark without the 
patch.

--
Added file: http://bugs.python.org/file35241/issue19385_speed.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

Right, my benchmark was indeed flawed. Here are the new results on my machine:

Without the patch
# ./python -S -m timeit -n 10 -s "import dbm.dumb as dbm; 
d=dbm.open('x.dat', 'c')"  "len(d)"
10 loops, best of 3: 0.564 usec per loop

With the patch
# ./python -S -m timeit -n 10 -s "import dbm.dumb as dbm; 
d=dbm.open('x.dat', 'c')"  "len(d)"
10 loops, best of 3: 0.857 usec per loop

Even having an empty _verify_open in __len__ method leads to this:

# ./python -S -m timeit -n 10 -s "import dbm.dumb as dbm; 
d=dbm.open('x.dat', 'c')"  "len(d)"
10 loops, best of 3: 0.749 usec per loop

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Jim Jewett]

Jim Jewett added the comment:

Did you try 3.5 without the patch, in case the issue is not with his code?
On May 13, 2014 7:23 AM, "Serhiy Storchaka"  wrote:

>
> Serhiy Storchaka added the comment:
>
> Claudiu, your benchmark is broken, it measure a no-op.
>
> Correct benchmark:
>
> $ ./python -S -m timeit -n 10 -s "import dbm.dumb as dbm;
> d=dbm.open('x.dat', 'c')"  "len(d)"
>
> 3.4:  10 loops, best of 3: 2.56 usec per loop
> 3.5:  10 loops, best of 3: 4.17 usec per loop
>
> There is 60% regression.
>
> --
> status: closed -> open
>
> ___
> Python tracker 
> 
> ___
>

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Claudiu, your benchmark is broken, it measure a no-op.

Correct benchmark:

$ ./python -S -m timeit -n 10 -s "import dbm.dumb as dbm; 
d=dbm.open('x.dat', 'c')"  "len(d)"

3.4:  10 loops, best of 3: 2.56 usec per loop
3.5:  10 loops, best of 3: 4.17 usec per loop

There is 60% regression.

--
status: closed -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Roundup Robot]

Roundup Robot added the comment:

New changeset e8343cb98cc3 by Benjamin Peterson in branch '3.4':
make operations on closed dumb databases raise a consistent exception (closes 
#19385)
http://hg.python.org/cpython/rev/e8343cb98cc3

New changeset dbceba88b96e by Benjamin Peterson in branch 'default':
merge 3.4 (#19385)
http://hg.python.org/cpython/rev/dbceba88b96e

--
nosy: +python-dev
resolution:  -> fixed
stage: commit review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

On my machine I get the following results for the unclosed-database case.

With patch:

# ./python -S -m timeit -n 10 -s "import dbm.dumb as dbm; 
d=dbm.open('x.dat', 'c');len(d)"
10 loops, best of 3: 0.0638 usec per loop

Without patch:

# ./python -S -m timeit -n 10 -s "import dbm.dumb as dbm; 
d=dbm.open('x.dat', 'c');len(d)"
10 loops, best of 3: 0.0634 usec per loop

--
Added file: http://bugs.python.org/file35016/issue19385_1.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Jim Jewett]

Jim Jewett added the comment:

I think the requested timing regression was for the non-broken case.  When the 
database has NOT been closed, and keys() still works, will it be way slower 
than before?

Note that I am not asking you to do that test (though the eventual committer 
might); the implementation of whichdb leaves me believing that almost anyone 
who is likely to care will have already followed the docunmentation's 
recommendation to install another *dbm ahead of dumb.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

gzip uses the same name, _check_closed, but your suggestion sounds good. I'll 
update the patch.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Jim Jewett]

Jim Jewett added the comment:

_check_closed sounds like you expect it to be closed, and might even assert 
that it is closed, except that you want the check run even in release mode 
and/or it might fail.  Since being closed is actually the unexpectedly broken 
state, I would prefer that you rename it to something else, like _verify_open.

--
nosy: +Jim.Jewett

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Zachary Ware]

Changes by Zachary Ware :


--
versions: +Python 3.5 -Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

Can this patch be committed, now that 3.5 is active?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

Updated the patch to catch dbm.dumb.error in tests (reverting a change added in 
c2f1bb56760d).

--
Added file: http://bugs.python.org/file34337/issue19385.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

If we agree that this should be fixed in 3.4, can somebody commit it before the 
second beta?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Nick Coghlan]

Nick Coghlan added the comment:

That's slightly more acceptable, but it's still hard to make the case
that changing exception types is a reasonable thing to do in a
maintenance release (it's only the specific nature of the error that
makes me consider it reasonable even in a feature release).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Arfrever Frehtes Taifersar Arahesis]

Arfrever Frehtes Taifersar Arahesis added the comment:

> in the last ever 3.3 release.

msg204533 suggests that there will be >=2 bug fix releases in 3.3 branch (3.3.4 
soon and 3.3.5 around release of 3.4.0).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Nick Coghlan]

Nick Coghlan added the comment:

I think it's definitely still OK to fix this in 3.4, but I think it's 
borderline enough to avoid including in the last ever 3.3 release. Changing 
exception types always introduces a little backwards compatibility risk, so 
it's something I lean towards only doing in new feature releases, even in cases 
like this where the old exception was clearly not the best choice.

--
versions: +Python 3.4 -Python 3.5

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> Now this seems odd, maybe catching + reraising an exception has a greater 
> overhead than a func call and checking if an attribute is None.

Yes, of course, catching + reraising an exception is costly. But when an 
exception is not raised, this is cheap. Usually len() is called for open 
database.

> IMHO it is a bug fix, not a new feature, and could be applied in 3.3 and 3.4.

Hmm. If consider dbm.dumb as separated module, this is perhaps a new feature. 
But if consider it as an implementation of the dbm protocol, it should conform 
to other dbm modules, and this is a bugfix. What would you say Nick?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Arfrever Frehtes Taifersar Arahesis]

Arfrever Frehtes Taifersar Arahesis added the comment:

IMHO it is a bug fix, not a new feature, and could be applied in 3.3 and 3.4.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

Here's a benchmark:

- with current patch:


# ./python -S -m timeit -n 1 -s 'import dbm.dumb as dbm; 
d=dbm.open("x.dat", "c");d.close()' 'try:' '  len(d)' 'except OSError:' '  pass'
1 loops, best of 3: 1.78 usec per loop


- using 
 try:
 return len(self._index)
 except TypeError:
 raise error('...')


# ./python -S -m timeit -n 1 -s 'import dbm.dumb as dbm; 
d=dbm.open("x.dat", "c");d.close()' 'try:' '  len(d)' 'except OSError:' '  pass'
1 loops, best of 3: 3.27 usec per loop


Now this seems odd, maybe catching + reraising an exception has a greater 
overhead than a func call and checking if an attribute is None. Anyway, I agree 
that dbm.dumb is not performance critical.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

> Regarding the first issue, I don't believe that the result will be as 
> readable (but I agree with you that it will be better).

Yes, perhaps performance of the dbm.dumb module is not worst this cost. Are you 
have any benchmarking results?

The patch LGTM.

> Should this be targeted for 3.5?

I think yes.

--
stage:  -> commit review
versions: +Python 3.5 -Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

Serhiy, what's the status for this? Should this be targeted for 3.5?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Changes by Claudiu.Popa :


Added file: http://bugs.python.org/file32453/dbm_dumb1.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Claudiu.Popa added the comment:

Here's the new version which addresses your last comment. Regarding the first 
issue, I don't believe that the result will be as readable (but I agree with 
you that it will be better). For instance, `items` will probably look like this:

try: 
   return [(key, self[key]) for key in self._index.keys()]
except AttributeError:
   raise dbm.dumb.error(...) from None

but will look totally different for other __len__:

try:
   return len(self._index)
except TypeError:
   raise dbm.dumb.error(...) from None.


We could catch TypeError only for dunder methods though and for the rest of the 
methods check the value of _index before access.

--
Added file: http://bugs.python.org/file32449/dbm_dumb1.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Yet one nitpick. I think that closing check should be after argument type check 
and key encoding.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Additional checks can slowdown the code. It doesn't matter in some operations, 
but not on __len__, __contains__, etc. EAFP approach is to catch AttributeError 
and raise appropriate dbm.dumb.error exception.

--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Nick Coghlan]

Nick Coghlan added the comment:

Patch looks good to me, I'll apply it when I apply issue 19282.

--
assignee:  -> ncoghlan
nosy: +ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Arfrever Frehtes Taifersar Arahesis]

Changes by Arfrever Frehtes Taifersar Arahesis :


--
nosy: +Arfrever

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

Changes by Claudiu.Popa :


--
keywords: +patch
Added file: http://bugs.python.org/file32347/dbm_dumb.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19385] dbm.dumb should be consistent when the database is closed

[Claudiu.Popa]

New submission from Claudiu.Popa:

This problem occurred in issue19282. Basicly, dbm.dumb is not consistent with 
dbm.gnu and dbm.ndbm when the database is closed, as seen in the following:


>>> import dbm.dumb as d
>>> db = d.open('test.dat', 'c')
>>> db.close()
>>> db.keys()
Traceback (most recent call last):
  File "", line 1, in 
  File "/tank/libs/cpython/Lib/dbm/dumb.py", line 212, in keys
return list(self._index.keys())
AttributeError: 'NoneType' object has no attribute 'keys'
>>>

vs

>>> import dbm.gnu as g
>>> db = g.open('test.dat', 'c')
>>> db.close()
>>> db.keys()
Traceback (most recent call last):
  File "", line 1, in 
_gdbm.error: GDBM object has already been closed
>>>

Attaching a patch.

--
components: Library (Lib)
messages: 201209
nosy: Claudiu.Popa
priority: normal
severity: normal
status: open
title: dbm.dumb should be consistent when the database is closed
type: enhancement
versions: Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com