636655 - Creating large textnodes became slower since 1.9.2

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Try the attached testcase.  It's 2x slower on trunk than in 1.9.2.

This looks like a regression from bug 582852, given my profiles and when this slowed down.

Trunk shows a lot more memory churn than 1.9.2, of course.  But worse yet, it scans the entire string on every creation!  And not only that, but the scan is _slower_ than the "is this ascii?" scan we used to do.

In particular, the scan is triggered by nsGenericDOMDataNode::SetTextInternal calling UpdateBidiStatus, which calls nsTextFragment::UpdateBidiFlag if the document doesn't already have RTL chars in it, and this last does:

  if (mState.mIs2b && !mState.mIsBidi) {
    // Check for surogates and bidi chars and stuff
  }

So we traded off doing a scan for ASCII on long strings for a scan for bidi chars (unless the document has some already).  This doesn't seem like a win to me....

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oh, and if you change the 12 in the testcase to a 9, we end up faster on 2.0 than in 1.9.2, of course.

Ehsan, what did you test the patch in bug 582852 on?  At the moment, it looks to me like we should just back it out...

Comment 3

[(no longer active)]

(In reply to comment #2)
> Oh, and if you change the 12 in the testcase to a 9, we end up faster on 2.0
> than in 1.9.2, of course.
> 
> Ehsan, what did you test the patch in bug 582852 on?  At the moment, it looks
> to me like we should just back it out...

I had a number of test case files for bug 240933 when I was working on it, but I can't find any of them around, unfortunately.  It's possible that the HTML file has had bidi chars in it (although I don't think that's likely -- I usually simplify my test cases as much as possible).  I need to investigate further, but so far it seems that you're right and we should back out the patch in bug 582852...

Comment 4

[Johnny Stenback (:jst)]

I wouldn't block on this, but drivers would certainly consider approving a backout if that's what we want to do here.

Comment 5

[(no longer active)]

Attachment: Patch (v1)

Yes, we should indeed back this out.  I did a bunch of tests with textareas, and we're actually experiencing a bunch of issues in large textareas based on how large they are, and there is definitely a bunch of work to be done in that area post 2.0, but this backout doesn't make anything worse, and it helps in cases such as the testcase Boris attached.

Comment 6

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/f787a4548826