1393348 - Optimize retrieving selection from nsTextEditorState

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I see nsTextInputSelectionImpl::GetSelection(RawSelectionType, nsISelection**) and nsFrameSelection::GetSelection() in the profile at running attachment 8848015 [details].

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebccc383cdb6aa899a5be63473b717353052834f

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5625cea02e4eb70a36e4f00dbc19ebf8937c8909

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7955505f14b5ae2a70bdc5449fc5d0ece101d6e1

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d104d093e83e3b4f7f4129a2945e67df1d112b1b

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1127354c870fdb5a2fd93de287cc79b24cedfd99

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=052c92712f5d5f13cb3f758f79a4acb23f32e8d5

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1393348 - part1: nsTextEditorState should use nsTextInputSelectionImpl::GetSelection(SelectionType) instead of nsTextInputSelectionImpl::GetSelection(RawSelectionType, nsISelection**)

nsTextEditorState stores selection controller as
RefPtr<nsTextInputSelectionImpl> mSelCon.  However, some methods still use
nsTextInputSelectionImpl::GetSelection(RawSelectionType, nsISelection**) which
is a virtual method overriding nsISelectionController.

So, instead, we should make it use
nsTextInputSelectionImpl::GetSelection(SelectionType).

Review commit: https://reviewboard.mozilla.org/r/172194/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172194/

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

nsISelectionController::SELECTION_* are declared as bit-mask.  However, no
methods of nsISelectionController treat them as bit-mask and these
values need a switch statement in nsFrameSelection to convert SelectionType to
array index of nsFrameSelection::mDOMSelections because it's too big to create
an array to do it.  Additionally, this conversion appears profile of
attachment 8848015 [details].

So, now, we should declare these values as sequential integer values.

However, only nsTextFrame uses these values as bit-mask.  Therefore, this patch
adds new type, SelectionTypeMask and creates new inline method,
ToSelectionTypeMask(SelectionType), to retrieve mask value for a SelectionType.

Review commit: https://reviewboard.mozilla.org/r/172196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172196/

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1393348 - part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement

GetIndexFromSelectionType() in nsFrameSelection.cpp or
nsFrameSelection::GetSelection() appears in profile of attachment 8848015 [details].  So,
it should not use switch statement due to really hot path.

With the previous patch, we can make it use array to retrieve the index from
SelectionType with static_cast<int8_t>.

Review commit: https://reviewboard.mozilla.org/r/172198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172198/

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8900970 [details]
Bug 1393348 - part1: nsTextEditorState should use nsTextInputSelectionImpl::GetSelection(SelectionType) instead of nsTextInputSelectionImpl::GetSelection(RawSelectionType, nsISelection**)

https://reviewboard.mozilla.org/r/172194/#review177708

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8900972 [details]
Bug 1393348 - part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement

https://reviewboard.mozilla.org/r/172198/#review177888

::: layout/generic/nsFrameSelection.cpp:165
(Diff revision 1)
> +};
> +
> +inline int8_t
>  GetIndexFromSelectionType(SelectionType aSelectionType)
>  {
> -  switch (aSelectionType) {
> +  return kIndexOfSelections[static_cast<int8_t>(aSelectionType) + 1];

I don't understand this. SelectionType NORMAL is 1, so we get index 2 here, no?
Were you going to use - 1?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

https://reviewboard.mozilla.org/r/172196/#review177890

::: dom/base/nsISelectionController.idl:309
(Diff revision 1)
> +
> +// SelectionType should be used in internal handling because of type safe.
>  enum class SelectionType : RawSelectionType
>  {
>    eInvalid = -1,
> -  eNone = nsISelectionController::SELECTION_NONE,
> +  // nsISelectionController::SELECTION_NONE

why you don't assign the value explicitly anymore? SELECTION_NONE is 0 and so is eNone.

::: layout/generic/nsTextFrame.cpp:7744
(Diff revision 1)
>  
>    UniquePtr<SelectionDetails> details = GetSelectionDetails();
>    for (SelectionDetails* sd = details.get(); sd; sd = sd->mNext.get()) {
>      if (sd->mStart == sd->mEnd ||
> -        !(sd->mSelectionType & kRawSelectionTypesWithDecorations) ||
> +        sd->mSelectionType == SelectionType::eInvalid ||
> +        !(sd->mSelectionType & kSelectionTypesWithDecorations) ||

Why is this right. mSelectionType isn't in bitmask form.

::: layout/generic/nsTextFrame.cpp:7822
(Diff revision 1)
>    nsPresContext* presContext = PresContext();
>    while (f && f->GetContentOffset() < int32_t(aEnd)) {
>      // We may need to reflow to recompute the overflow area for
>      // spellchecking or IME underline if their underline is thicker than
>      // the normal decoration line.
> -    if (aSelectionType & kRawSelectionTypesWithDecorations) {
> +    if (aSelectionType & kSelectionTypesWithDecorations) {

Why is this right? aSelection type isn't in bitmask form.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8900972 [details]
Bug 1393348 - part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement

https://reviewboard.mozilla.org/r/172198/#review177888

> I don't understand this. SelectionType NORMAL is 1, so we get index 2 here, no?
> Were you going to use - 1?

The array is storing from index of eInvalid (-1). Therefore, index for eNormal is stored at 2. That the reason why I added +1.

I'd like to get rid of if/switch from the method.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

https://reviewboard.mozilla.org/r/172196/#review177890

> why you don't assign the value explicitly anymore? SELECTION_NONE is 0 and so is eNone.

Okay. I'll revert the changes.

> Why is this right. mSelectionType isn't in bitmask form.

Because of SelectionType has custom & operator override. See Selection.h.

Or, should I create SelectionTypeIsIncluded(SelectionType, SelectionTypeMask)? But it may make other developers confused which is SelectionType and SelectionTypeMask. I we could write it as SelectionTypeMask::Includes(SelectionType)...

> Why is this right? aSelection type isn't in bitmask form.

See above. The & operator is overridden in Selection.h (declared in nsISelectionController.idl).

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

setting ni? because I need reply for the above before I rewrote the patches.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8900972 [details]
Bug 1393348 - part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement

https://reviewboard.mozilla.org/r/172198/#review178108

oh, I see. Could you add some comment.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

https://reviewboard.mozilla.org/r/172196/#review178114

::: dom/base/Selection.h:582
(Diff revision 1)
> +  MOZ_ASSERT(aSelectionType != SelectionType::eInvalid);
> +  return aSelectionType == SelectionType::eNone ? 0 :
> +           (1 << (static_cast<uint8_t>(aSelectionType) - 1));
> +}
> +
>  inline bool operator &(SelectionType aSelectionType,

Could we get rid of this operator& and just explicitly call ToSelectionTypeMask when needed.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

https://reviewboard.mozilla.org/r/172196/#review178114

> Could we get rid of this operator& and just explicitly call ToSelectionTypeMask when needed.

Ah, I should've gone to the bed for refreshing my brain :-(

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2cef3edcc430bd5d16cd7c60b3f96b3f870fc4d

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8900970 [details]
Bug 1393348 - part1: nsTextEditorState should use nsTextInputSelectionImpl::GetSelection(SelectionType) instead of nsTextInputSelectionImpl::GetSelection(RawSelectionType, nsISelection**)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172194/diff/1-2/

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172196/diff/1-2/

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8900972 [details]
Bug 1393348 - part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172198/diff/1-2/

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8900971 [details]
Bug 1393348 - part2: nsISelectionController::SELECTION_* should be sequential integer values

https://reviewboard.mozilla.org/r/172196/#review178310

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc9c5f963abf
part1: nsTextEditorState should use nsTextInputSelectionImpl::GetSelection(SelectionType) instead of nsTextInputSelectionImpl::GetSelection(RawSelectionType, nsISelection**) r=m_kato
https://hg.mozilla.org/integration/autoland/rev/a69d5ede21ba
part2: nsISelectionController::SELECTION_* should be sequential integer values r=smaug
https://hg.mozilla.org/integration/autoland/rev/156c3f4e0b16
part3: Converting SelectionType to index of Selection array in nsFrameSelection should use array instead of switch statement r=smaug

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fc9c5f963abf
https://hg.mozilla.org/mozilla-central/rev/a69d5ede21ba
https://hg.mozilla.org/mozilla-central/rev/156c3f4e0b16