Summary: | Scipt causes KHTML to freeze | ||
---|---|---|---|
Product: | [Applications] konqueror | Reporter: | Frans Leerink <f.leerink> |
Component: | khtml | Assignee: | Konqueror Developers <konq-bugs> |
Status: | RESOLVED FIXED | ||
Severity: | crash | CC: | maksim |
Priority: | NOR | ||
Version: | unspecified | ||
Target Milestone: | --- | ||
Platform: | unspecified | ||
OS: | Linux | ||
Latest Commit: | Version Fixed In: | ||
Sentry Crash Report: | |||
Attachments: |
patch
updated patch |
Description
Frans Leerink
2006-06-01 01:24:50 UTC
KJS is much slower than SpiderMonkey. So long JavaScript scripts will make Konqueror slow. That's what that message is showing. That is no bug. And, besides, the webpage works fine here. (1)Please don't spread performance myths. KHTML/KJS and Gecko/SpiderMonkey performance is generally comparable. (2)This has nothing to do with speed of KJS, but rather with speed of a text-escaping function in KHTML, which does tons of memory allocations per-character. (3)Slowness is legitimate bug. In case where (1) doesn't hold, it usually means something really stupid is happening. ... And in this case, the "other thing" is absolutely horrific implementation of innerHTML (in particular escapeHTML) which means there are tons of calls that take like 200ms each if I slow my machine down to 600 MHz)... Created attachment 16483 [details]
patch
OK, this makes innerHTML on the page about 10x-20x faster, and makes things
much smoother. Unfortunately, I am not 100% confident in safety of the
DOMString change --- it could do funny things if someone is sharing a
DOMStringImpl but not ref'ing, though I hope that sort of thing isn't
happening; but I don't have time to test/commit this anyway...
Ugh. Except after a few more tries, it's only 2x faster. I blame dialup for lack of reproducibility. Looks like tons of mess inside ElementImpl::openTagStartToString, etc. DOMString just doesn't work for this... Created attachment 16495 [details]
updated patch
This one I feel a lot safer about
SVN commit 556347 by orlovich: Make reading of innerHTML a couple times less slow BUG:128397 M +5 -3 html/html_elementimpl.cpp M +4 -3 xml/dom_elementimpl.cpp M +40 -0 xml/dom_stringimpl.cpp M +2 -0 xml/dom_stringimpl.h M +1 -19 xml/dom_textimpl.cpp --- branches/KDE/3.5/kdelibs/khtml/html/html_elementimpl.cpp #556346:556347 @@ -450,9 +450,11 @@ DOMString HTMLElementImpl::innerHTML() const { - DOMString result = ""; - for (NodeImpl *child = firstChild(); child != NULL; child = child->nextSibling()) - result += child->toString(); + QString result; //Use QString to accumulate since DOMString is poor for appends + for (NodeImpl *child = firstChild(); child != NULL; child = child->nextSibling()) { + DOMString kid = child->toString(); + result += QConstString(kid.unicode(), kid.length()).string(); + } return result; } --- branches/KDE/3.5/kdelibs/khtml/xml/dom_elementimpl.cpp #556346:556347 @@ -824,17 +824,18 @@ DOMString ElementImpl::toString() const { - DOMString result = openTagStartToString(); + QString result = openTagStartToString().string(); //Accumulate in QString, since DOMString can't append well. if (hasChildNodes()) { result += ">"; for (NodeImpl *child = firstChild(); child != NULL; child = child->nextSibling()) { - result += child->toString(); + DOMString kid = child->toString(); + result += QConstString(kid.unicode(), kid.length()).string(); } result += "</"; - result += tagName(); + result += tagName().string(); result += ">"; } else if (result.length() == 1) { // ensure we dont get results like < /> can happen when serialize document --- branches/KDE/3.5/kdelibs/khtml/xml/dom_stringimpl.cpp #556346:556347 @@ -416,5 +416,45 @@ return QConstString(s, i).string().toInt(ok); } +static const unsigned short amp[] = {'&', 'a', 'm', 'p', ';'}; +static const unsigned short lt[] = {'&', 'l', 't', ';'}; +static const unsigned short gt[] = {'&', 'g', 't', ';'}; +DOMStringImpl *DOMStringImpl::escapeHTML() +{ + unsigned outL = 0; + for (unsigned int i = 0; i < l; ++i ) { + if ( s[i] == '&' ) + outL += 5; //& + else if (s[i] == '<' || s[i] == '>') + outL += 4; //>/< + else + ++outL; + } + if (outL == l) + return this; + + DOMStringImpl* toRet = new DOMStringImpl(); + toRet->s = QT_ALLOC_QCHAR_VEC(outL); + toRet->l = outL; + + unsigned outP = 0; + for (unsigned int i = 0; i < l; ++i ) { + if ( s[i] == '&' ) { + memcpy(&toRet->s[outP], amp, sizeof(amp)); + outP += 5; + } else if (s[i] == '<') { + memcpy(&toRet->s[outP], lt, sizeof(lt)); + outP += 4; + } else if (s[i] == '>') { + memcpy(&toRet->s[outP], gt, sizeof(gt)); + outP += 4; + } else { + toRet->s[outP] = s[i]; + ++outP; + } + } + return toRet; +} + --- branches/KDE/3.5/kdelibs/khtml/xml/dom_stringimpl.h #556346:556347 @@ -74,6 +74,7 @@ return new DOMStringImpl(s,l); }; + DOMStringImpl *substring(unsigned int pos, unsigned int len); DOMStringImpl *collapseWhiteSpace(bool preserveLF, bool preserveWS); @@ -89,6 +90,7 @@ DOMStringImpl *lower() const; DOMStringImpl *upper() const; DOMStringImpl *capitalize(bool noFirstCap=false) const; + DOMStringImpl *escapeHTML(); QChar *unicode() const { return s; } uint length() const { return l; } --- branches/KDE/3.5/kdelibs/khtml/xml/dom_textimpl.cpp #556346:556347 @@ -39,25 +39,7 @@ static DOMString escapeHTML( const DOMString& in ) { - //FIXME: this is rather slow - DOMString s; - for ( unsigned int i = 0; i < in.length(); ++i ) { - switch( in[i].latin1() ) { - case '&': - s += "&"; - break; - case '<': - s += "<"; - break; - case '>': - s += ">"; - break; - default: - s += DOMString( in[i] ); - } - } - - return s; + return in.implementation()->escapeHTML(); } CharacterDataImpl::CharacterDataImpl(DocumentPtr *doc, DOMStringImpl* _text) |