I'd like to request some clarifications on the recently checked dict patch. How it is supposed to work and why is this solution okay?
What's the exact purpose of the 2nd string specialization patch?
Besides that, I must say that now the interpreter is noticeably slower and MAL and I were warning you kindly about this code, which was fine tuned over the years. It is very sensible and was optimized to death. The patch that did make it was labeled "not ready" and I would have appreciated another round of review. Not that I disagree, but now I feel obliged to submit another patch to make some obvious perf improvements (at least), which simply duplicates work... Fred would have done them very well, but I haven't had the time to say much about the implementation because the laconic discussion on the Patch Manager went about functionality.
Now I'd like to bring this on python-dev and see what exactly happened to lookdict and what the BeOpen team agreed on regarding this function.
Vladimir Marangozov wrote:
I'd like to request some clarifications on the recently checked dict patch. How it is supposed to work and why is this solution okay?
What's the exact purpose of the 2nd string specialization patch?
Besides that, I must say that now the interpreter is noticeably slower and MAL and I were warning you kindly about this code, which was fine tuned over the years. It is very sensible and was optimized to death. The patch that did make it was labeled "not ready" and I would have appreciated another round of review. Not that I disagree, but now I feel obliged to submit another patch to make some obvious perf improvements (at least), which simply duplicates work... Fred would have done them very well, but I haven't had the time to say much about the implementation because the laconic discussion on the Patch Manager went about functionality.
Now I'd like to bring this on python-dev and see what exactly happened to lookdict and what the BeOpen team agreed on regarding this function.
Just for the record:
Python 1.5.2: 3050 pystones Python 2.0b1: 2850 pystones before the lookup patch 2900 pystones after the lookup patch My old considerably patched Python 1.5: 4000 pystones
I like Fred's idea about the customized and auto-configuring lookup mechanism. This should definitely go into 2.1... perhaps even with a hook that allows C extensions to drop in their own implementations for certain types of dictionaries, e.g. ones using perfect hash tables.
Thanks, Marc-Andre, for pointing out that Fred's lookdict code is actually an improvement.
The reason for all this is that we found that lookdict() calls PyObject_Compare() without checking for errors. If there's a key that raises an error when compared to another key, the keys compare unequal and an exception is set, which may disturb an exception that the caller of PyDict_GetItem() might be calling. PyDict_GetItem() is documented as never raising an exception. This is actually not strong enough; it was actually intended to never clear an exception either. The potential errors from PyObject_Compare() violate this contract. Note that these errors are nothing new; PyObject_Compare() has been able to raise exceptions for a long time, e.g. from errors raised by __cmp__().
The first-order fix is to call PyErr_Fetch() and PyErr_restore() around the calls to PyObject_Compare(). This is slow (for reasons Vladimir points out) even though Fred was very careful to only call PyErr_Fetch() or PyErr_Restore() when absolutely necessary and only once per lookdict call. The second-order fix therefore is Fred's specialization for string-keys-only dicts.
There's another problem: as fixed, lookdict needs a current thread state! (Because the exception state is stored per thread.) There are cases where PyDict_GetItem() is called when there's no thread state! The first one we found was Tim Peters' patch for _PyPclose (see separate message). There may be others -- we'll have to fix these when we find them (probably after 2.0b1 is released but hopefully before 2.0 final).
--Guido van Rossum (home page: http://www.pythonlabs.com/~guido/)
Aha. Thanks for the explanation.
Guido van Rossum wrote:
Thanks, Marc-Andre, for pointing out that Fred's lookdict code is actually an improvement.
Right. I was too fast. There is some speedup due to the string specialization. I'll post a patch to SF with some more tweaks of this implementation. Briefly:
- do not call PyErr_Clear() systematically after PyObject_Compare(); only if (!error_restore && PyErr_Occurred()) - defer variable initializations after common return cases - avoid using more vars in lookdict_string + specialize string_compare() - inline the most frequest case in PyDict_GetItem (the first item probe)
The reason for all this is that we found that lookdict() calls PyObject_Compare() without checking for errors. If there's a key that raises an error when compared to another key, the keys compare unequal and an exception is set, which may disturb an exception that the caller of PyDict_GetItem() might be calling. PyDict_GetItem() is documented as never raising an exception. This is actually not strong enough; it was actually intended to never clear an exception either. The potential errors from PyObject_Compare() violate this contract. Note that these errors are nothing new; PyObject_Compare() has been able to raise exceptions for a long time, e.g. from errors raised by __cmp__().
The first-order fix is to call PyErr_Fetch() and PyErr_restore() around the calls to PyObject_Compare(). This is slow (for reasons Vladimir points out) even though Fred was very careful to only call PyErr_Fetch() or PyErr_Restore() when absolutely necessary and only once per lookdict call. The second-order fix therefore is Fred's specialization for string-keys-only dicts.
There's another problem: as fixed, lookdict needs a current thread state! (Because the exception state is stored per thread.) There are cases where PyDict_GetItem() is called when there's no thread state! The first one we found was Tim Peters' patch for _PyPclose (see separate message). There may be others -- we'll have to fix these when we find them (probably after 2.0b1 is released but hopefully before 2.0 final).
Hm. Question: is it possible for the thread state to swap during PyObject_Compare()? If it is possible, things are more complicated than I thought...
Right. I was too fast. There is some speedup due to the string specialization. I'll post a patch to SF with some more tweaks of this implementation. Briefly:
- do not call PyErr_Clear() systematically after PyObject_Compare(); only if (!error_restore && PyErr_Occurred())
What do you mean? The lookdict code checked in already checks PyErr_Occurrs().
- defer variable initializations after common return cases
- avoid using more vars in lookdict_string + specialize string_compare()
- inline the most frequest case in PyDict_GetItem (the first item probe)
Cool.
Hm. Question: is it possible for the thread state to swap during PyObject_Compare()? If it is possible, things are more complicated than I thought...
Doesn't matter -- it will always swap back. It's tied to the interpreter lock.
Now, for truly devious code dealing with the lock and thread state, see the changes to _PyPclose() that Tim Peters just checked in...
--Guido van Rossum (home page: http://www.pythonlabs.com/~guido/)
Guido van Rossum wrote:
Right. I was too fast. There is some speedup due to the string specialization. I'll post a patch to SF with some more tweaks of this implementation. Briefly:
- do not call PyErr_Clear() systematically after PyObject_Compare(); only if (!error_restore && PyErr_Occurred())
What do you mean? The lookdict code checked in already checks PyErr_Occurrs().
Was fast again. Actually PyErr_Clear() is called on PyErr_Occurred(). PyErr_Occurred() is called systematically after PyObject_Compare() and it will evaluate to true even if the error was previously fetched.
So I mean that the test for detecting whether a *new* exception is raised by PyObject_Compare() is (!error_restore && PyErr_Occurred()) because error_restore is set only when there's a previous exception in place (before the call to Object_Compare). And only in this case we need to clear the new error.
- do not call PyErr_Clear() systematically after PyObject_Compare(); only if (!error_restore && PyErr_Occurred())
What do you mean? The lookdict code checked in already checks PyErr_Occurrs().
Was fast again. Actually PyErr_Clear() is called on PyErr_Occurred(). PyErr_Occurred() is called systematically after PyObject_Compare() and it will evaluate to true even if the error was previously fetched.
No, PyErr_Fetch() clears the exception! PyErr_Restore() restores it.
--Guido van Rossum (home page: http://www.pythonlabs.com/~guido/)
Guido van Rossum wrote:
- do not call PyErr_Clear() systematically after PyObject_Compare(); only if (!error_restore && PyErr_Occurred())
What do you mean? The lookdict code checked in already checks PyErr_Occurrs().
Was fast again. Actually PyErr_Clear() is called on PyErr_Occurred(). PyErr_Occurred() is called systematically after PyObject_Compare() and it will evaluate to true even if the error was previously fetched.
No, PyErr_Fetch() clears the exception! PyErr_Restore() restores it.
Oops, right. This saves a function call, then. Still good.
participants (3)
-
Guido van Rossum -
M.-A. Lemburg -
Vladimir.Marangozov＠inrialpes.fr