A lot of callers of nsRange::SetStart() calls nsRange::SetEnd() immediately. Then, nsRange::SetEnd() needs redundant processing. Therefore, nsRange should have a method to set both of them once. Now, nsRange::Set() is that, but that is just calling SetStart() and SetEnd(). So, we should optimize it and rename it to better name, perhaps, SetStartAndEnd(). The coming patch changes a lot of callers. So, I'd like to land this in 55 since such change makes harder to uplift when you work on security bugs.

The patch improves the score of attachment 8848015 [details] (bug 1346723) 3%~.

Comment on attachment 8871187 [details] Bug 1367683 Optimize initializing nsRange https://reviewboard.mozilla.org/r/142670/#review146332 Looks like mostly DOM stuff- I'm punting the review to smaug instead.

I agree with the change in general though - nice perf improvement!

Comment on attachment 8871187 [details] Bug 1367683 Optimize initializing nsRange https://reviewboard.mozilla.org/r/142670/#review147688 ::: dom/base/nsRange.h:172 (Diff revision 1) > + * the start point or the end point is invalid point. > + * If the specified start point is after the end point, the range will be > + * collapsed at the end point. Similarly, if they are in different root, > + * the range will be collapsed at the end point. > + */ > + nsresult SetStartAndEnd(nsINode* aParent, int32_t aOffset) I don't understand how this method works. It takes just parent and offset. How is that setting both start and end? Could we call this CollapseTo(aParent, aOffset) ::: dom/base/nsRange.h:176 (Diff revision 1) > + */ > + nsresult SetStartAndEnd(nsINode* aParent, int32_t aOffset) > { > - // If this starts being hot, we may be able to optimize this a bit, > - // but for now just set start and end separately. > - nsresult rv = SetStart(aStartParent, aStartOffset); > + return SetStartAndEnd(aParent, aOffset, aParent, aOffset); > + } > + nsresult SetStartAndEnd(nsINode* aStartParent, int32_t aStartOffset, This variant is easy to understand ::: dom/base/nsRange.h:183 (Diff revision 1) > > - return SetEnd(aEndParent, aEndOffset); > + /** > + * Retrieves node and offset for setting start or end of a range to > + * before or after aNode. > + */ > + static nsINode* GetNodeAndOffsetAfter(nsINode* aNode, int32_t* aOffset) Shouldn't this be called GetParentAndOffsetAfter ::: dom/base/nsRange.h:194 (Diff revision 1) > + if (*aOffset >= 0) { > + (*aOffset)++; > + } > + return parentNode; > + } > + static nsINode* GetNodeAndOffsetBefore(nsINode* aNode, int32_t* aOffset) GetParentAndOffsetBefore ::: dom/base/nsRange.cpp:1275 (Diff revision 1) > return; > } > > AutoInvalidateSelection atEndOfBlock(this); > - aRv = SetStart(aNode.GetParentNode(), IndexOf(&aNode)); > + int32_t offset = -1; > + nsINode* node = GetNodeAndOffsetBefore(&aNode, &offset); s/node/parent/ ::: dom/base/nsRange.cpp:1311 (Diff revision 1) > return; > } > > AutoInvalidateSelection atEndOfBlock(this); > - aRv = SetStart(aNode.GetParentNode(), IndexOf(&aNode) + 1); > + int32_t offset = -1; > + nsINode* node = GetNodeAndOffsetAfter(&aNode, &offset); s/node/parent/ ::: dom/base/nsRange.cpp:1465 (Diff revision 1) > return; > } > > AutoInvalidateSelection atEndOfBlock(this); > - aRv = SetEnd(aNode.GetParentNode(), IndexOf(&aNode)); > + int32_t offset = -1; > + nsINode* node = GetNodeAndOffsetBefore(&aNode, &offset); s/node/parent/ ::: dom/base/nsRange.cpp:1501 (Diff revision 1) > return; > } > > AutoInvalidateSelection atEndOfBlock(this); > - aRv = SetEnd(aNode.GetParentNode(), IndexOf(&aNode) + 1); > + int32_t offset = -1; > + nsINode* node = GetNodeAndOffsetAfter(&aNode, &offset); s/node/parent/ ::: dom/events/ContentEventHandler.cpp:988 (Diff revision 1) > *aLastTextNode = nullptr; > } > > // Special case like <br contenteditable> > if (!mRootContent->HasChildren()) { > - nsresult rv = aRange->SetStart(mRootContent, 0); > + nsresult rv = aRange->SetStartAndEnd(mRootContent, 0); CollapseTo ::: dom/events/ContentEventHandler.cpp:2882 (Diff revision 1) > if (!childNode || !childNode->IsNodeOfType(nsINode::eTEXT) || > NS_WARN_IF(offsetInChildNode < 0)) { > return NS_OK; > } > > - nsresult rv = aRange->Set(childNode, offsetInChildNode, > + nsresult rv = aRange->SetStartAndEnd(childNode, offsetInChildNode, Why is this using the 4 param version. Couldn't this be CollapseTo ::: editor/libeditor/HTMLEditRules.cpp:1457 (Diff revision 1) > + curNode, curOffset); > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > } else { > - rv = mDocChangeRange->SetEnd(selNode, selOffset); > + rv = mDocChangeRange->SetStartAndEnd(selNode, selOffset, This could be CollapseTo, right? ::: editor/libeditor/HTMLEditRules.cpp:8342 (Diff revision 1) > if (!mListenerEnabled) { > return NS_OK; > } > + nsCOMPtr<nsINode> rightNode = do_QueryInterface(aRightNode); > // assumption that Join keeps the righthand node > - nsresult rv = mUtilRange->SetStart(aRightNode, mJoinOffset); > + nsresult rv = mUtilRange->SetStartAndEnd(rightNode, mJoinOffset); CollapseTo ::: editor/libeditor/HTMLEditRules.cpp:8394 (Diff revision 1) > { > if (!mListenerEnabled) { > return NS_OK; > } > - nsCOMPtr<nsIDOMNode> theNode = do_QueryInterface(aTextNode); > - nsresult rv = mUtilRange->SetStart(theNode, aOffset); > + nsCOMPtr<nsINode> theNode = do_QueryInterface(aTextNode); > + nsresult rv = mUtilRange->SetStartAndEnd(theNode, aOffset); CollapseTo ::: extensions/spellcheck/src/mozInlineSpellChecker.cpp:1222 (Diff revision 1) > - rv = range->SetEndAfter(aEndNode); > - NS_ENSURE_SUCCESS(rv, rv); > + return rv; > + } > + } else { > + int32_t endOffset = -1; > + nsCOMPtr<nsINode> startNode = do_QueryInterface(aStartNode); > + nsCOMPtr<nsINode> endNode = do_QueryInterface(aEndNode); Couldn't you move nsCOMPtrs to be before if, then there would be less copy-pasting. ::: layout/generic/nsSelection.cpp:1798 (Diff revision 1) > // non-anchor/focus collapsed ranges. > mDomSelections[index]->RemoveCollapsedRanges(); > > RefPtr<nsRange> newRange = new nsRange(aNewFocus); > > - newRange->SetStart(aNewFocus, aContentOffset); > + newRange->SetStartAndEnd(aNewFocus, aContentOffset); CollapseTo ::: layout/generic/nsSelection.cpp:5244 (Diff revision 1) > } > } > } > > RefPtr<nsRange> range = new nsRange(parentNode); > - result = range->SetEnd(parentNode, aOffset); > + result = range->SetStartAndEnd(parentNode, aOffset); CollapseTo

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/6736d54ffd89 Optimize initializing nsRange r=smaug