If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Cache the frame for caret

RESOLVED WORKSFORME

Status

()

Core
Layout
RESOLVED WORKSFORME
7 years ago
7 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({perf})

Trunk
mozilla2.0b4
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

We really need to make sure that we cache the computation results of nsLineBox::IndexOf as a frame property.  This would win us another 15% on the editing performance of large textareas.  This should be easy (and similar to bug 580869).  The only thing that I'm not sure of is when that cache should be cleared.  Roc, bz, do you have any ideas?
The return value of IndexOf depends on the line, no? 

And in this case we're actually trying to optimize the results of findlinecontaining, right?

I would be fine with caching an nsLineBox* for the last FindLineContaining result and then starting the next search at that line.  We'd need to clear the cache when lines go away (possibly only when the cached line does, if it's cheap enough to do that test).
We already have a line cursor on blocks. Can we use that?
That line cursor has various limitations in it in terms of what it can be used for... and is invalidated on any frametree change.  Might be ok for editor's purposes, though.
I don't really know anything about the line cursor.  Could you please point me to some relevant code so I can start learning my way through that code?
(In reply to comment #1)
> The return value of IndexOf depends on the line, no? 

I'm not sure what you mean.

> And in this case we're actually trying to optimize the results of
> findlinecontaining, right?

Not necessarily.
> I'm not sure what you mean.

IndexOf returns the index of a given frame in a given line (possibly -1 if the frame is not in that line).  This operation is really only slow if we have lots of frames in the line, which usually only happens if we have lots of placeholders.  The other reason IndexOf shows up in profiles is if we're calling IndexOf on _lots_ of lines.  At that point, the call costs (can we inline it?) and other overhead add up.

As I understand, you'd like to optimize the latter case, right?  What do you propose to cache?
I was actually misreading the profiles.  We're indeed making lots of nsLineBox::IndexOf calls, each of which takes very little time to finish.  I think a good way to mitigate this problem is to make the caret code (which is the largest caller of IndexOf in this case) cache the frame and frame offset that it has found instead of trying to compute them over and over.
Summary: Optimize nsLineBox::IndexOf → Cache the frame for caret
Created attachment 460972 [details] [diff] [review]
Patch (v1)

With this patch, nsLineBox::IndexOf is completely removed from the profiles when pressing and holding down a key (or typing very quickly).  With this patch and the rest of the patches, we're 70-80% faster than what we were before the optimizations that I've made.  I'm continuing to profile different cases and see what other gains we can get.
Attachment #460972 - Flags: review?(roc)
Attachment #460972 - Flags: review?(roc) → review+
Attachment #460972 - Flags: approval2.0?
Attachment #460972 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/4ee2f3cf11d0
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
Blocks: 585922
No longer blocks: 585922
Depends on: 585922
Depends on: 593211
It should be noted that the code parts of this bug was backed out as part of bug 593211, which implemented a better optimization.
Resolution: FIXED → WONTFIX
Resolution: WONTFIX → WORKSFORME
You need to log in before you can comment on or make changes to this bug.