Closed Bug 755480 Opened 9 years ago Closed 9 years ago

Optimize WordSplitState::FindSpecialWord some more

Categories

(Core :: Spelling checker, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: perf)

Attachments

(3 files, 2 obsolete files)

Bug 753233 fixed the suckiness of this function for data: URIs, but this function still stinks perf-wise.

1. It goes through some effort to compute foundDot, and then doesn't use it!
2. When it finds a colon, it keeps on looking at every other character, when it can just look at the next character, and bail out if it detects a URL-like thingy.
3. Worst of all, for strings without @ (and : if number 2 above is fixed), this is O(nm) where n is the number of characters in the string, and m is the number of words in the string.  This is because the caller hopelessly calls this over and over *even* if it returns -1, which means, "hey caller, there's nothing interesting anywhere in this string!".

Point #3 just caused me to have to kill Firefox after waiting a few minutes for it to hit the event loop again.  This is unacceptable!
Attached patch Part 1Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #624170 - Flags: review?(roc)
Attached patch Part 2Splinter Review
Attachment #624172 - Flags: review?(roc)
Attached patch Part 3 (obsolete) — Splinter Review
This is the most valuable optimization here: don't call FindSpecialWord repeatedly if it returns -1 in an iteration.

Note that this is mostly reindenting existing code.
Attachment #624173 - Flags: review?(roc)
Comment on attachment 624173 [details] [diff] [review]
Part 3

Review of attachment 624173 [details] [diff] [review]:
-----------------------------------------------------------------

I think this would be clearer if FindSpecialWord simply became IsSpecialWord, returning bool, and SplitDOMWord just did AdvanceThroughSeparators() first, then called IsSpecialWord, and if not a special word then did the loop.
Attached patch Part 3 (obsolete) — Splinter Review
Attachment #624173 - Attachment is obsolete: true
Attachment #624173 - Flags: review?(roc)
Attachment #624453 - Flags: review?(roc)
Comment on attachment 624453 [details] [diff] [review]
Part 3

Review of attachment 624453 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/spellcheck/src/mozInlineSpellWordUtil.cpp
@@ +1009,5 @@
> +
> +    // skip the special word
> +    state.mDOMWordOffset += specialWordLength;
> +    if (state.mDOMWordOffset + aStart >= aEnd)
> +      state.mCurCharClass = CHAR_CLASS_END_OF_INPUT;

I think state.mDOMWordOffset is always equal to aEnd here. That means we can simplify the code. In fact, it's very confusing to have dead code here.
Attached patch Part 3Splinter Review
Attachment #624453 - Attachment is obsolete: true
Attachment #624453 - Flags: review?(roc)
Attachment #624742 - Flags: review?(roc)
Comment on attachment 624742 [details] [diff] [review]
Part 3

Review of attachment 624742 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that fixed.

::: extensions/spellcheck/src/mozInlineSpellWordUtil.cpp
@@ +1007,5 @@
> +    mRealWords.AppendElement(
> +        RealWord(aStart + state.mDOMWordOffset, specialWordLength, false));
> +
> +    // skip the special word
> +    state.mDOMWordOffset += specialWordLength;

No point in updating 'state' since it's local to this function and we're about to return!
Attachment #624742 - Flags: review?(roc) → review+
Duplicate of this bug: 782359
You need to log in before you can comment on or make changes to this bug.