speed improvements (patch)

[Martin Aumueller]

Thanks for your your on Amarok so far. Yesterday, I actually had the 
impression that it has become a lot snappier...

But the description of your work on AtomicString makes me think that this 
would render AtomicString pretty much useless: the reason for introducing it 
was to save memory by holding only one copy of an artist's name instead of 
one separate for each track. But if you do QDeepCopys, then this advantage is 
lost. Please correct me if I'm wrong, but if I understood you correctly then 
I propose to get rid of AtomicString altogether, as it just introduces 
unnecessary complexity.


On Thu October 5 2006 07:30, Ovidiu Gheorghioiu wrote:
> Thanks for the warm welcome guys!
>
> I was getting ready to split the patch into multiple files like Mark
> suggested, but it seems it's no longer necessary. Yippee, first
> contribution to Amarok :)
>
> Next up -- while looking at AtomicString out of curiosity, I've
> discovered that it wasn't really thread safe, for a number of reasons.
> First up, the mutex was not static, like the set it was protecting.
> Second, some refs and derefs were done un-protected. So I wrote a
> small program to unit test it, that simply allocates and deallocates
> some strings in two threads (without communicating explicitly, but
> their contents match a lot so they trigger the AtomicString
> single-store optimization). Then I spent a chunk of time making it not
> crash.
>
> It harder than I expected, mainly because of QString's inherent thread
> unsafety -- it tries not to reallocate on assignment, but without
> locks, which was a surprise to me. So we have to store deep copies in
> the AtomicString store and deep copy them when we take them out.
>
> I've attached a patch (including also the unittest that crashed it
> before, 100% on my dual-core) that makes it work well in multiple
> threads. It's not optimal, in that the single global lock, which is
> also held over refs / derefs, might some day become a performance
> problem (I've verified that right now it isn't) but at least it's
> safe. And while QString is also unsafe, at least it's only unsafe when
> an instance is explicitly used between threads, but AtomicString was
> unsafe even on unrelated instances, like in my unit test, that just
> happen to use the same text.
>
> If anybody has a better solution, that doesn't crash the unit test and
> does not involve a mutex per string, I'd be happy to see it.
>
> Regards,
> Ovy
>
> On 10/4/06, Alexandre Oliveira <aleprjlists at gmail.com> wrote:
> > I commited it, as the changes indeed made a lot of sense.
> > I just replaced the qFatal by a warning (we don't use qFatal ever),
> > and fixed the check that would lead to it.
> > _______________________________________________
> > Amarok mailing list
> > Amarok at kde.org
> > https://mail.kde.org/mailman/listinfo/amarok