[issue17389] Optimize Event.wait()

[Antoine Pitrou]

Antoine Pitrou added the comment:

Agreed.

--
resolution:  -> rejected
status: open -> closed

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Mark Dickinson]

Mark Dickinson added the comment:

[Antoine]
> That said, I agree the performance improvement is probably not important
> here. It was just drive-by optimization on my part :-)

Shall we close this as rejected?

--

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Charles-François Natali]

Charles-François Natali added the comment:

> Would that be erroneous? It can already happen because of thread switches:

That's not really the same thing. wait() would return True, which is
the right thing to do since the Event has been set at some point.
Here, it would make it possible for wait() to return immediately after
another thread clear()ed the Event, which would be just wrong.

> Charles-François: would your objections also apply to the current 
> implementation of 'is_set'?

Yes (in OpenJDK CountDownLatch implementation, a similar method uses a
volatile variable to guarantee memory visibility.)

> For implementations with a GIL the GIL ensures there are no visibility
> issues.

Agreed.

> According to Antoine's link, Jython uses ConcurrentHashMap for its
dicts, and IronPython uses a locked map.  So in those cases also there
are no visibility issues.

Agreed.

But the question remains whether that's something which is part of the
memory model: I'm skeptical about this, since it would pretty much
imply memory barriers everywhere and disable a whole class of
caching/hoisting, which would just kill performance.

--

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Richard Oudkerk]

Richard Oudkerk added the comment:

On 22/03/2013 3:31pm, Charles-François Natali wrote:
>> I was under the impression that dict access (and therefore attribute
>> access for "simple" objects) was guaranteed to be atomic even in
>> alternative implementations like Jython and IronPython.
>>
>> Is this not a language guarantee?
>
> AFAICT there's no such guarantee.
> But even if dict access/assignment was guaranteed to be atomic, this
> wouldn't solve the visibility issue.

"Atomic" was maybe the wrong word.

For implementations with a GIL the GIL ensures there are no visibility 
issues.

According to Antoine's link, Jython uses ConcurrentHashMap for its 
dicts, and IronPython uses a locked map.  So in those cases also there 
are no visibility issues.

--

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Mark Dickinson]

Mark Dickinson added the comment:

Charles-François: would your objections also apply to the current 
implementation of 'is_set'?

--

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Antoine Pitrou]

Antoine Pitrou added the comment:

> So in short, if wait() is called by a thread shortly after another
> thread clear()ed it, the former thread might very well read _flag ==
> True (while the later just set it to False) and return erroneously.

Would that be erroneous? It can already happen because of thread switches:
e.g. wait() reads True from the flag, then releases the internal lock;
another thread gets scheduled in, takes the lock and resets the event;
the original thread is resumed and the calling function sees True as
the wait() return value even though the event was reset in-between.

> Now, it's probably being pedantic, especially because we lack a
> memory
> model, but that bothers me.

As for the memory model, Jeffrey had written a PEP draft years ago:
http://code.google.com/p/unladen-swallow/wiki/MemoryModel

Unfortunately I can't find the corresponding reference in the python-dev
archives again.

That said, I agree the performance improvement is probably not important
here. It was just drive-by optimization on my part :-)

--
nosy: +jyasskin

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Charles-François Natali]

Charles-François Natali added the comment:

> I was under the impression that dict access (and therefore attribute
> access for "simple" objects) was guaranteed to be atomic even in
> alternative implementations like Jython and IronPython.
>
> Is this not a language guarantee?

AFAICT there's no such guarantee.
But even if dict access/assignment was guaranteed to be atomic, this
wouldn't solve the visibility issue.

--

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Richard Oudkerk]

Richard Oudkerk added the comment:

On 22/03/2013 3:19pm, Charles-François Natali wrote:
> The _flag is checked without any lock held: although it won't be a
> problem with CPython, a standard memory model (e.g. Java's one)
> doesn't guarantee that reading _flag outside of the lock will return
> the value most recently written to it (because of caching/hoisting, or
> store buffers/invalidate queues at CPU level).
>
> So in short, if wait() is called by a thread shortly after another
> thread clear()ed it, the former thread might very well read _flag ==
> True (while the later just set it to False) and return erroneously.

I was under the impression that dict access (and therefore attribute 
access for "simple" objects) was guaranteed to be atomic even in 
alternative implementations like Jython and IronPython.

Is this not a language guarantee?

--

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Charles-François Natali]

Charles-François Natali added the comment:

Something bothers me:
"""
def wait(self, timeout=None):
if self._flag:
return True

self._cond.acquire()
"""

The _flag is checked without any lock held: although it won't be a
problem with CPython, a standard memory model (e.g. Java's one)
doesn't guarantee that reading _flag outside of the lock will return
the value most recently written to it (because of caching/hoisting, or
store buffers/invalidate queues at CPU level).

So in short, if wait() is called by a thread shortly after another
thread clear()ed it, the former thread might very well read _flag ==
True (while the later just set it to False) and return erroneously.

Now, it's probably being pedantic, especially because we lack a memory
model, but that bothers me.

Also, I'm not sure this is really a hot-path.

--

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[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

[issue17389] Optimize Event.wait()

[Antoine Pitrou]

Antoine Pitrou added the comment:

> How about the attached simpler patch?

Looks better indeed! I had completely overlooked the possible race
condition.

--

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Mark Dickinson]

Mark Dickinson added the comment:

How about the attached simpler patch?

--
Added file: http://bugs.python.org/file29364/event_wait_metd.patch

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Mark Dickinson]

Mark Dickinson added the comment:

I think there's a race condition in the patched version:  'self._flag' could be 
set in between if 'if not signaled:' and the 'self._cond.acquire()', and in 
that case you'll end up waiting for an event that's already occurred.  Do you 
need to recheck 'self._flag' after acquiring the condition?

--

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Mark Dickinson]

Changes by Mark Dickinson :


--
nosy: +mark.dickinson

___
Python tracker 

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

[issue17389] Optimize Event.wait()

[Antoine Pitrou]

New submission from Antoine Pitrou:

It should be possible to optimize threading.Event.wait() to not acquire the 
condition when the event is already set. Patch attached.

Without patch:

$ ./python -m timeit -s "import threading; e = threading.Event(); e.set()" 
"e.wait()"
100 loops, best of 3: 0.466 usec per loop

With patch:

$ ./python -m timeit -s "import threading; e = threading.Event(); e.set()" 
"e.wait()"
1000 loops, best of 3: 0.19 usec per loop

--
components: Library (Lib)
files: event_wait.patch
keywords: patch
messages: 183811
nosy: neologix, pitrou, sbt
priority: normal
severity: normal
stage: patch review
status: open
title: Optimize Event.wait()
type: performance
versions: Python 3.4
Added file: http://bugs.python.org/file29355/event_wait.patch

___
Python tracker 

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