[Webkit-unassigned] [Bug 34489] [Qt] Text codec lookup is slow

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34489





--- Comment #3 from David Leong <david.leong at nokia.com>  2010-02-02 13:00:42 PST ---
Thanks for the comments, I will rework this as suggested by Laszlo and Ariya.

QTextCodec internally already maintains a cache of text codecs. The performance
problem is caused by the fact that QTextCodec has to check for the name and
aliases associated with each codec in order to return from their cache. The
proposed webkit local cache is for usage locality when we request the same
codec over and over again. (I'll also log something to Qt if appropriate)

(In reply to comment #2)
> (From update of attachment 47948 [details])
> Hmm, seems the proper fix is to optimize QTextCodec.
> 
> Anyway, some comments:
> 
> > -    m_codec = QTextCodec::codecForName(m_encoding.name());
> > +    // Very likely to reuse the same text codecs in frequent order,
> > +    // QTextCodec is very slow with matching encoding name to codec, so we make a cache here
> > +    // 
> > +    const char* name = m_encoding.name();
> > +    QString qName = QString::fromAscii(name);
> 
> This creates the unnecessary deep copy of name in the heap. However, I don't
> think using QLatin1String would work either since you potentially insert the
> encoding name later on to the cache.

David: I originally wanted to use QString( char* ) to not deep copy the data
but the API has been deprecate in Qt 4.6

> 
> > +    TextCodecHash::iterator it = staticTextCodecCache.find(qName);
> > +    if( it != staticTextCodecCache.end() ) {
> > +        m_codec = it.value();
> > +        
> > +    }
> > +    else {
> > +        m_codec = QTextCodec::codecForName(m_encoding.name());
> > +        staticTextCodecCache.insert(qName, m_codec);
> > +    }
> 
> A more readable version would be to use QHash::value(). Maybe something like:
> 
> m_codec = staticTextCodecCache.value(qName);
> if (!m_coded) {
>         m_codec = QTextCodec::codecForName(m_encoding.name());
>        staticTextCodecCache.insert(qName, m_codec);
> }
> 
> Also, any particular reason why the codec cache can't be a static member in
> TextCodecQt class instead of a global one like that?

David: OK, i will make the cache a static member in TextCodecQt and rework the
other code.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.