[issue15275] isinstance is called a more times that needed in ntpath

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
assignee:  -> serhiy.storchaka
resolution:  -> fixed
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

[issue15275] isinstance is called a more times that needed in ntpath

[Roundup Robot]

Roundup Robot added the comment:

New changeset b22aaa59d24f by Serhiy Storchaka in branch 'default':
Issue #15275: Clean up and speed up the ntpath module.
http://hg.python.org/cpython/rev/b22aaa59d24f

--
nosy: +python-dev

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[STINNER Victor]

STINNER Victor added the comment:

Oh you're right sorry.

--

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

No, if *path* is not bytes, *userhome* shouldn't be converted to bytes.

--

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[STINNER Victor]

STINNER Victor added the comment:

I like ntpath_cleanup.diff, I don't think that it makes the code worse.

FYI os.fsencode() accepts str too, you can simplify:

 if isinstance(path, bytes):
-userhome = userhome.encode(sys.getfilesystemencoding())
+userhome = os.fsencode(userhome)

to


+userhome = os.fsencode(userhome)

--
nosy: +haypo

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Here is alternative patch. I believe it makes a code simpler.

Microbenchmarks:

$ ./python -m timeit -n 10 -s "from ntpath import splitdrive"  
"splitdrive('c:foo')"

Before: 10 loops, best of 3: 20 usec per loop
After: 10 loops, best of 3: 11.5 usec per loop

$ ./python -m timeit -n 10 -s "from ntpath import splitext"  
"splitext('python.exe')"

Before: 10 loops, best of 3: 23.6 usec per loop
After: 10 loops, best of 3: 18 usec per loop

$ ./python -m timeit -s "from ntpath import join"  "join('foo', 'bar')"

Before: 1 loops, best of 3: 50.9 usec per loop
After: 1 loops, best of 3: 32.3 usec per loop

$ ./python -m timeit -s "from ntpath import normpath"  
"normpath('/foo/bar/baz')"

Before: 1 loops, best of 3: 67.5 usec per loop
After: 1 loops, best of 3: 40.3 usec per loop

$ ./python -m timeit -s "from ntpath import relpath"  "relpath('foo', 'bar')"

Before: 1000 loops, best of 3: 695 usec per loop
After: 1000 loops, best of 3: 456 usec per loop

--
nosy: +serhiy.storchaka
versions: +Python 3.5 -Python 3.4
Added file: http://bugs.python.org/file36030/ntpath_cleanup.diff

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Mark Lawrence]

Mark Lawrence added the comment:

@Manuel do you intend picking this up?

--
nosy: +BreamoreBoy

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Ezio Melotti]

Ezio Melotti  added the comment:

> I don't know about a decent way of doing benchmarks for the changes.
> Any recommendation?

You could make a script that uses the timeit module.

>> If this patch is applied I think it would be good to change
>> posixpath too.
> I agree and I'd love to do it but in a diff bug to make things
> self-contained, what do you think?

Having a single patch that fixes both is OK.

--

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Manuel de la Pena]

Manuel de la Pena  added the comment:

Tests indeed cover the changes made. I don't know about a decent way of doing 
benchmarks for the changes. Any recommendation?

> If this patch is applied I think it would be good to change posixpath too.

I agree and I'd love to do it but in a diff bug to make things self-contained, 
what do you think?

--

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Terry J. Reedy]

Changes by Terry J. Reedy :


--
nosy: +terry.reedy

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Ezio Melotti]

Ezio Melotti  added the comment:

Have you tried doing some benchmarks before and after the patch?
If this patch is applied I think it would be good to change posixpath too.
Also make sure that the changes you made are covered by the tests.

--
nosy: +ezio.melotti
stage:  -> patch review
type:  -> performance
versions: +Python 3.4 -Python 3.3

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Manuel de la Pena]

Changes by Manuel de la Pena :


--
nosy: +brian.curtin

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Jesús Cea Avión]

Changes by Jesús Cea Avión :


--
nosy: +jcea

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Manuel de la Pena]

Changes by Manuel de la Pena :


Removed file: http://bugs.python.org/file26295/f5c57ba1124b.diff

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Manuel de la Pena]

Changes by Manuel de la Pena :


Added file: http://bugs.python.org/file26295/f5c57ba1124b.diff

___
Python tracker 

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

[issue15275] isinstance is called a more times that needed in ntpath

[Manuel de la Pena]

New submission from Manuel de la Pena :

The problem is simple, the code that allows to use binary strings and unicode 
is making more calls that needed to isinstance(path, bytes) since the result of 
the code is not shared. For example, the following calls are present in the 
module:

def _get_empty(path):
if isinstance(path, bytes):
return b'' 
else:
return ''

def _get_sep(path):
if isinstance(path, bytes):
return b'\\'
else:
return '\\'

def _get_altsep(path):
if isinstance(path, bytes):
return b'/'
else:
return '/'

def _get_bothseps(path):
if isinstance(path, bytes):
return b'\\/'
else:
return '\\/'

def _get_dot(path):
if isinstance(path, bytes):
return b'.'
else:
return '.'

...

And then something similar to the following is found in the code:

def normpath(path):
"""Normalize path, eliminating double slashes, etc."""
sep = _get_sep(path)
dotdot = _get_dot(path) * 2
special_prefixes = _get_special(path)
if path.startswith(special_prefixes):
# in the case of paths with these prefixes:
# \\.\ -> device names
# \\?\ -> literal paths
# do not do any normalization, but return the path unchanged
return path
path = path.replace(_get_altsep(path), sep)
prefix, path = splitdrive(path)

As you can see the isinstance call is performed more than needed which 
certainly affects the performance of the path operations. 

The attached patch removes the number of calls to isinstance(obj, bytes) and 
also ensures that the function that returns the correct literal is as fast as 
possible by using a dict.

--
components: Windows
files: less_isinstance.patch
hgrepos: 140
keywords: patch
messages: 164842
nosy: mandel
priority: normal
severity: normal
status: open
title: isinstance is called a more times that needed in ntpath
versions: Python 3.3
Added file: http://bugs.python.org/file26294/less_isinstance.patch

___
Python tracker 

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