classification
Title: check_GetFinalPathNameByHandle() suboptimal
Type: performance Stage: resolved
Components: Extension Modules Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: brian.curtin, catalin.iacob, ishimoto, pitrou, python-dev, sijinjoseph
Priority: low Keywords: easy, patch

Created on 2011-05-08 20:00 by pitrou, last changed 2012-10-21 14:42 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue12034.patch ishimoto, 2012-07-27 04:00 review
Messages (8)
msg135540 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-08 20:00
The code for check_GetFinalPathNameByHandle() goes like this:


static int has_GetFinalPathNameByHandle = 0;
[...]

static int
check_GetFinalPathNameByHandle()
{
    HINSTANCE hKernel32;
    /* only recheck */
    if (!has_GetFinalPathNameByHandle)
    {
        [...]
        has_GetFinalPathNameByHandle = Py_GetFinalPathNameByHandleA &&
                                       Py_GetFinalPathNameByHandleW;
    }
    return has_GetFinalPathNameByHandle;
}


It means that if the computed value is 0 (system doesn't have GetFinalPathNameByHandle*), the value will be re-computed each time the function is called.  has_GetFinalPathNameByHandle should probably be tri-state instead (0, 1, unknown).
msg136123 - (view) Author: Sijin Joseph (sijinjoseph) Date: 2011-05-16 20:00
Is this related to some other issue? The fix seems trivial, however I am curious as to how you stumbled upon this?

Is there more to this issue than just performance?
msg136124 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-05-16 20:02
Yes, it should be trivial to fix.
I've stumbled on this simply because I needed to write similar code for another patch, so studied how it was done in posixmodule.c.
msg136789 - (view) Author: Catalin Iacob (catalin.iacob) * Date: 2011-05-24 20:19
I looked at providing a patch for this issue as an introductory step (this would be my first patch).

But when following the code I discovered that the case that this issue is trying to optimize is never executed in current CPython. In that case, there isn't much value in optimizing it.

More precisely, check_GetFinalPathNameByHandle is called by posix__getfinalpathname which is nt._getfinalpathname in Python code. If the check fails, posix__getfinalpathname throws NotImplmenentedError. But nt._getfinalpathname is only used by ntpath.py which checks the Windows version and only calls nt._getfinalpathname for Vista and higher where the check won't fail.

To me it would make more sense that the nt module has a _getfinalpathname attribute only if it supports the feature instead of always having one that throws NotImplementedError. In that case, ntpath.py would not check the Windows version but the presence of _getfinalpathname in the nt module. Does this seem like a better approach to you, at least theoretically? And if so, is it worth implementing it?

Thanks for any advice.
msg166540 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2012-07-27 04:00
Patch to cache result of check_GetFinalPathNameByHandle().
msg166545 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-27 06:45
Thank you, patch looks fine to me. It will have to wait for after the 3.3 release, though, since we are now in feature freeze.
msg173451 - (view) Author: Roundup Robot (python-dev) Date: 2012-10-21 14:35
New changeset 8afa3ce5ff3e by Antoine Pitrou in branch 'default':
Issue #12034: Fix bogus caching of result in check_GetFinalPathNameByHandle.
http://hg.python.org/cpython/rev/8afa3ce5ff3e
msg173452 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-10-21 14:42
Patch committed, thank you!
History
Date User Action Args
2012-10-21 14:42:00pitrousetstatus: open -> closed
resolution: fixed
messages: + msg173452

stage: resolved
2012-10-21 14:35:37python-devsetnosy: + python-dev
messages: + msg173451
2012-07-27 06:45:23pitrousetmessages: + msg166545
versions: + Python 3.4, - Python 3.2, Python 3.3
2012-07-27 04:00:22ishimotosetfiles: + issue12034.patch

nosy: + ishimoto
messages: + msg166540

keywords: + patch
2011-05-24 20:19:19catalin.iacobsetnosy: + catalin.iacob
messages: + msg136789
2011-05-16 20:02:58pitrousetmessages: + msg136124
2011-05-16 20:00:31sijinjosephsetmessages: + msg136123
2011-05-11 11:39:34sijinjosephsetnosy: + sijinjoseph
2011-05-08 20:00:05pitroucreate