Currently, if we detect the presence of any RTL content on the page, we mark the document as requiring bidi processing, and this results in nsBidiPresUtils::Resolve being called for all blocks; this collects the text content of the block (recursively including all its subframes) into a string buffer, and then calls ResolveParagraph to process it. There are often cases, however, where the majority of blocks on a page do not really need any bidi resolution; they contain purely LTR text, with no directional overrides, bidi embeddings, etc., and if the document as a whole had not been marked (due to an RTL character somewhere else) as requiring bidi processing, we would skip all this work. So what I'd like to do here is to optimize away some bidi resolution, in cases where we can determine that a block doesn't require it. We'll still need to scan the contents of the block -- to find out whether it has any bidi content -- but if it doesn't, then we don't really need to copy all the text into a string buffer to pass to the bidi engine, or execute the full bidi resolution and reordering algorithm. This should save some time on pages where only a smattering of bidi content is present in an otherwise LTR document; for example, an English page that happens to cite an isolated Arabic example somewhere, or that contains a list of links to other-language versions, each in its native script.

This is preliminary to the main patch here: I needed a better HasRTLChars() than the rough-and-ready version currently in nsBidiUtils, and noticed that we had what should be equivalent functionality (except that it's more accurate) inlined in nsTextFragment::UpdateBidiFlag. So this patch moves it from there to nsBidiUtils so we can use it more generally, replacing the current code there (which would return too many false positives these days, e.g. for all the supplementary-plane emoji characters).

And here's the real patch. The idea here is to return early from nsBidiPresUtils::Resolve, without doing all the work of bidi resolution, in the (common) case where no directional overrides/embeddings are in effect, and recursively traversing the contents shows no RTL characters present.

To see the effect of this, I profiled loading a large hg changeset as mentioned in bug 542877 comment 4: http://hg.mozilla.org/mozilla-central/rev/25caa0ec50f2. Profile with current trunk code: https://perfht.ml/2pWITk7 With these patches: https://perfht.ml/2pk6UW9 This indicates the total time spent in nsBidiPresUtils code is reduced from 1310ms to 511ms for that example.

Fix stupid error in a last-minute update to the patch :( -- sorry for the bugspam.

Comment on attachment 8860196 [details] [diff] [review] Preliminary cleanup: refactor nsTextFragment::UpdateBidiFlag(), moving its logic into nsBidiUtils function HasRTLChars() and making that function more precise in the process Review of attachment 8860196 [details] [diff] [review]: ----------------------------------------------------------------- I admit to not understanding the old nsBidiUtils::HasRTLChars() impl (with its magic numbers). I imagine it'd make some sense after I dug into the magic numbers -- but per comment 1 it sounds like it wasn't really correct anyway, so I didn't bother. I did verify that the migrated code (from nsTextFragment.cpp) seems equivalent from its old vs. new home. Given that & my confidence that you know what you're doing, r=me

Comment on attachment 8860185 [details] [diff] [review] Skip the main body of bidi-resolution for blocks that can be determined to be purely LTR content without directional overrides/embeddings Review of attachment 8860185 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsBidiPresUtils.cpp @@ +1287,5 @@ > +bool > +nsBidiPresUtils::PreTraverseFrames(nsIFrame* aCurrentFrame, > + nsIContent** aCurrContent) > +{ > + if (!aCurrentFrame) { Perhaps add an assertion like the following at the beginning of this function: MOZ_ASSERT(!aCurrentFrame || !aCurrentFrame->GetPrevSibling(), "Expecting to traverse from the start of a child list"); @@ +1293,5 @@ > + } > + > + nsIFrame* childFrame = aCurrentFrame; > + do { > + nsIFrame* nextSibling = childFrame->GetNextSibling(); I think you could condense the first 6 lines of this function (along with the "while" check at the end of this do{...}while loop) if you just reformulated this as an equivalent "for" loop: for (nsIFrame* childFrame = aCurrentFrame; childFrame; childFrame = childFrame->GetNextSibling()) { Shorter & more understandable (IMO). @@ +1300,5 @@ > + if (nsGkAtoms::placeholderFrame == childFrame->GetType()) { > + nsIFrame* realFrame = > + nsPlaceholderFrame::GetRealFrameForPlaceholder(childFrame); > + if (realFrame->GetType() == nsGkAtoms::letterFrame) { > + frame = realFrame; Can you add some documentation here? I don't understand why placeholders are special here, nor why specifically-firstletter placeholders are extra-special. Really, a brief explanatory comment for each chunk of this while loop would be helpful, to hint at what we're checking for... At least, that'd be helpful for me as someone who's not super-familiar with bidi, and I expect for future bidi-non-experts who read this code. ::: layout/base/nsBidiPresUtils.h @@ +400,5 @@ > BidiParagraphData* aBpd); > > /** > + * A "pre-traversal" of the child frames of the block element, > + * to determine whether full bidi resolution is actually needed. A few nits: (1) Please add the word "Perform" at the beginning of this sentence ('Perform a "pre-traversal" [...]), so that it's more of a sentence rather than a noun-phrase & will therefore be more consistent with other nearby documentation. (2) Please add documentation for the args here. (Particularly aCurrContent, since it's an in/out param I think, which means it's a bit noteworthy.) @@ +404,5 @@ > + * to determine whether full bidi resolution is actually needed. > + * Returns true if it finds that bidi is (or may be) required, > + * false if no potentially-bidi content is present. > + */ > + static bool PreTraverseFrames(nsIFrame* aCurrentFrame, Two nits on the function signature: (1) The function name doesn't sound very "bool-flavored" right now. (It sounds quite similar to TraverseFrames, which returns type 'void'.) How about we name it something slightly more self-explanatory like "ChildListMayRequireBidi"? (The documentation can still explain that it's a "pre-traversal" etc.) That makes its usages easier to understand -- it's obvious what "if (ChildListMayRequireBidi(...))" means, but it's not at all obvious what "if (PreTraverseFrames(...))" means. (2) "aCurrentFrame" seems like an odd name for this arg. That makes it sound like it's a loop counter or something -- but really it's expected to be the first frame in a child list, I think. (A child list off of "the block element" mentioned in the documentation.) So how about we name this "aFirstChild" or something like that? (in the .h and .cpp files)

Here's a substantially improved version, including renaming the "pre-traversal" method as suggested, and more cleanup.

Comment on attachment 8860771 [details] [diff] [review] Skip the main body of bidi-resolution for blocks that can be determined to be purely LTR content without directional overrides/embeddings Review of attachment 8860771 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits. ::: layout/base/nsBidiPresUtils.cpp @@ +1103,5 @@ > c->DrainSelfOverflowList(); > } > > + controlChar = GetBidiControl(sc); > + overrideChar = GetBidiOverride(sc); These optimizations inside TraverseFrames() (optimizing away repeated StyleContext calls) seem unrelated to the rest of this patch. Ideally it'd be great to split these into their own (somewhat trivial) patch -- perhaps "part 3" here -- so that the main patch is a bit easier to grok and to reason about in case of regressions. rs=me on that standalone patch if you want to split it out. @@ +1293,5 @@ > +nsBidiPresUtils::ChildListMayRequireBidi(nsIFrame* aFirstChild, > + nsIContent** aCurrContent) > +{ > + MOZ_ASSERT(!aFirstChild || !aFirstChild->GetPrevSibling(), > + "Expecting to traverse from the start of a child list"); Fix indentation here. @@ +1297,5 @@ > + "Expecting to traverse from the start of a child list"); > + > + if (!aFirstChild) { > + return false; > + } Perhaps get rid of this early-return -- it's kind of "pre-redundant" given the "for" loop that follows it. (It's among the lines that I was saying could be condensed in comment 6.) (If aFirstChild is null, then the "for" loop will immediately fail the for-loop's loop condition, and we'll jump straight to "return false" at the end of the function.) @@ +1334,5 @@ > + } > + } > + } > + } else { > + if (ChildListMayRequireBidi(frame->PrincipalChildList().FirstChild(), Consider simplifying else{if to "else if" ::: layout/base/nsBidiPresUtils.h @@ +406,5 @@ > + * This explores the same frames as TraverseFrames (above), but is less > + * expensive and may allow us to avoid performing the full TraverseFrames > + * operation. > + * @param aFirstChild frame to start traversal from > + * @param[in/out] aCurrContent most recently visited content node, This isn't quite true. If we traverse an empty block, for example, we don't update aCurrContent to point to that block frame's content node. Similarly, if GetBidiControl() or GetBidiOverride() return nonzero, we return true without ever updating aCurrContent. We only set aCurrContent for text frames that we decide to scan for RTL characters, AFAICT. How about: "the content node that we've most recently scanned for RTL characters (so that [...])"

One more piece needed here. The optimization in this bug means that we don't attach a BidiDataProperty to many frames even when bidi is enabled for the document. But if there's no property, nsIFrame::GetBidiData returns a record with all fields 0; this in turn means that the test in BuildTextRunsScanner::ContinueTextRunAcrossFrames unnecessarily breaks text-run merging because it believes aFrame2 had a preceding bidi control, which it didn't. By returning a record with precedingControl=kBidiLevelNone when the property is absent, we can avoid this regression.

Oops, there was a silly last-minute error in the patch just posted; now fixed.

I wonder whether we can put some flag to indicate whether a block is pure LTR or not. IIRC bidi resolution happens after reflow, and we should be able to collect all needed information during reflow. With that, we can avoid the additional traversal. Not sure whether it's worth a state flag + that code complexity, though.

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #11) > I wonder whether we can put some flag to indicate whether a block is pure > LTR or not. IIRC bidi resolution happens after reflow, That doesn't sound right in the general case... we need to do bidi resolution before we can create textruns for the contents of a block, and we need to create textruns before we can do line-breaking... and we need to do line-breaking during reflow. Hence, nsBlockFrame::ResolveBidi is called during the early part of nsBlockFrame::Reflow, before it proceeds to reflow its content.

Oh, hmmm, yep... We do reorder after reflow, but resolution needs to happen befote.

Comment on attachment 8861602 [details] [diff] [review] When no BidiDataProperty() is present, nsIFrame::GetBidiData() should return data with precedingControl=kBidiLevelNone to avoid unecessarily interrupting textruns Review of attachment 8861602 [details] [diff] [review]: ----------------------------------------------------------------- This seems fine; r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/99440ee674fe8dff6336119e3f3cc360e52301ca Bug 1358275 - Preliminary cleanup: refactor nsTextFragment::UpdateBidiFlag(), moving its logic into nsBidiUtils function HasRTLChars() and making that function more precise in the process. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/155ba7301e591b0bff8d90c4037aeefccf755ccf Bug 1358275 - Skip the main body of bidi-resolution for blocks that can be determined to be purely LTR content without directional overrides/embeddings. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/779ea6294a225a49a7f4787ad2145fc1bef8c5cd Bug 1358275 - When no BidiDataProperty() is present, nsIFrame::GetBidiData() should return data with precedingControl=kBidiLevelNone to avoid unecessarily interrupting textruns. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/033145d323135c10d4eb2780e18ffd7bbb17335b Bug 1358275 - Optimize away repeated StyleContext() calls in nsBidiPresUtils::TraverseFrames. rs=dholbert

https://hg.mozilla.org/mozilla-central/rev/99440ee674fe https://hg.mozilla.org/mozilla-central/rev/155ba7301e59 https://hg.mozilla.org/mozilla-central/rev/779ea6294a22 https://hg.mozilla.org/mozilla-central/rev/033145d32313