Closed Bug 636655 Opened 9 years ago Closed 9 years ago

Creating large textnodes became slower since 1.9.2

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- -

People

(Reporter: bzbarsky, Assigned: Ehsan)

References

(Blocks 1 open bug)

Details

(Keywords: perf, regression)

Attachments

(2 files)

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....
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...
blocking2.0: --- → ?
(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...
Assignee: nobody → ehsan
Blocks: 582852
I wouldn't block on this, but drivers would certainly consider approving a backout if that's what we want to do here.
blocking2.0: ? → -
Attached patch Patch (v1)Splinter Review
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.
Attachment #515239 - Flags: review?(bzbarsky)
Attachment #515239 - Flags: approval2.0?
Attachment #515239 - Flags: review?(bzbarsky)
Attachment #515239 - Flags: review+
Attachment #515239 - Flags: approval2.0?
Attachment #515239 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/f787a4548826
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
You need to log in before you can comment on or make changes to this bug.