756276 - Add a thread safe queue and port nsEventQueue as well as MediaQueue to use it

Status: UNCONFIRMED

(Core :: XPCOM, defect)

Description

[Doug Sherk (:drs) (inactive)]

For the Fennec panning and zooming code, a thread-safe queue is needed to queue up events but not the same way as nsEventQueue. Implementations of queues exist scattered around the code base, but none are really useful or work for all implementations.

MediaQueue: Works and is thread-safe but has random media stuff embedded in it. Also uses nsDeque as its base so it doesn't handle nsCOMPtrs and such correctly.
nsDeque: Works but isn't thread safe and uses void pointers everywhere, so it sucks for GC.
nsEventQueue: Works but can only take nsIRunnables. Is also heavily integrated with the event/thread code.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Preliminary patch, only with new additions (no other queues ported yet)

Still need to write code to port MediaQueue and nsEventQueue to this.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 624925 [details] [diff] [review]
Preliminary patch, only with new additions (no other queues ported yet)

Review of attachment 624925 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/TestThreadSafeQueue.cpp
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et cindent: */
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Nit: MPL2.

@@ +55,5 @@
> +static bool test_push()
> +{
> +  ThreadSafeQueue<PRUint32> queue(20);
> +  PRUint32 data[] = {1,2,3,4,5,6,7,8,9,10};
> +  for (PRUint32 i = 0; i < sizeof(data) / sizeof(PRUint32); i++) {

mozilla::ArrayLength, please.  Here and elsewhere.

Also, for tidyness's sake, you should use size_t i = 0.

@@ +247,5 @@
> +
> +static bool test_ptr_behavior() {
> +  ThreadSafeQueue<nsRefPtr<RefCountString> > queue;
> +
> +#define NUM_STRING_TESTS_QUEUE 5

mozilla::ArrayLength(data)

::: xpcom/threads/Makefile.in
@@ +64,5 @@
>  		LazyIdleThread.cpp \
>  		$(NULL)
>  
>  EXPORTS		= \
> +		ThreadSafeQueue.h \

Did you forget ThreadSafeQueue.h in the patch?

Comment 3

[Doug Sherk (:drs) (inactive)]

Attachment: Preliminary patch, only with new additions (no other queues ported yet)

New version. Yes, I forgot ThreadSafeQueue.h. I also fixed all other mentioned issues. I added a new test to TestThreadSafeQueue.cpp called test_thread_waiting(). It tests whether or not, when mayWait=true, a thread will actually wait until data is inserted into the queue.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

There's dom/workers/Queue.h too. Is it generic enough to be useful to you?

Comment 5

[Doug Sherk (:drs) (inactive)]

I wish someone had told me about that before. From looking through the code for it, though, it looks like it pops from the front of an nsTArray, which I believe necessitates a shift down of all elements. My queue is designed to just invalidate that section of memory and erase it later when there's either too much accumulated invalid memory or space is needed. I like the locking policy being templated in, though.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

No, it reverses the list it has accumulated and then pulls from the end, see http://mxr.mozilla.org/mozilla-central/source/dom/workers/Queue.h#78 

You pay the price for reversing, of course.

Seems easy enough to make a new StoragePolicy that does what you're describing though.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

An intern a couple years ago wrote a lock-free queue.  I would poke around for that work.

But TBH, we don't have much use for high-performance multi-threaded queues in Gecko currently.

Comment 8

[Josh Matthews [:jdm]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> An intern a couple years ago wrote a lock-free queue.  I would poke around
> for that work.

By the name of Roy Frostig, if I'm not mistaken.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Bug 500308 is what I was thinking of.  I don't know of anything Roy did.