Closed Bug 1290312 Opened 7 years ago Closed 7 years ago

Remove mMappedFlows from TextRunUserData

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: xidorn, Assigned: sumi29, Mentored)

References

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file)

Remove mMappedFlows from TextRunData - bug 1290312
58 bytes, text/x-review-board-request
xidorn
: review+
Details 
This point field almost always points to the memory immediately after the struct (the only place doesn't do so is in-stack).

If we do allocate TextRunUserData a lot, it may be worth saving one pointer of memory.
Mentor: ealvarez
Whiteboard: [good first bug][lang=c++]
I'm new and I like to work on this bug.But I did not have any idea to how to start to fix the bug.Can I know some more details?
We need to wait for bug 1288938 to land first. Also the patch there makes this bug more complicated.
Depends on: 1288938
Can I know the where is the bug is?
https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.h

In the struct above, the pointer mMappedFlows almost always points to the end of this struct.

The following places are how it is allocated:
https://dxr.mozilla.org/mozilla-central/rev/0ba72e8027cfcbcbf3426770ac264a7ade2af090/layout/generic/nsTextFrame.cpp#456-466
https://dxr.mozilla.org/mozilla-central/rev/0ba72e8027cfcbcbf3426770ac264a7ade2af090/layout/generic/nsTextFrame.cpp#476-487

That said, we don't really need this field, as its value can easily be computed. This bug is about removing this field. Since TextRunUserData could be allocated lots of times, a pointer size may be worth saving.

Emilio said he would like to mentor this bug, so let's ni? him.
Flags: needinfo?(ealvarez)
Yes, the idea is removing the mMappedFlows pointer for a function that computes it, so we save that space. It can be tricky, because if we do this we have to differentiate between |TextRunUserData| and |ComplexTextRunUserData| (using the textrun flags), because the offset is different.

So we want a function like |TextRunMappedFlow* GetMappedFlows(gfxTextRun* aTextRun, uint32_t* aOutCount)|, where we make this computation looking at the textrun flag TEXT_MIGHT_HAVE_GLYPH_CHANGES, and using it whenever we iterate over the mapped flows now.

Let me know if you need more information, happy to help! :)
Flags: needinfo?(ealvarez)
Thank you Xidorn, I am going to study on this.
Thank you Emilio, It's my first time contributing open source project so I'm very glad if someone can guide me to solve this..
Hi Prageeth, are you still working on it? Do you need any help? Thanks!
Flags: needinfo?(prageetheranga)
Yes Emilio, I am still studying the code. If I get a problem, i will ask your help.
@Emilio:
The SEGVs occur in the FindFlowForContent function; more specifically, on line 2680 of the patched version (the first if-condition). Build passes without any new warnings, ofcourse.
Inspecting the command in assembly tells me that I am trying to access a memory location that I probably shouldn't, but I can't seem to find out why exactly that is. 

Also, please ignore variable names and the general uncleanliness of the code, I'll clean that up as soon as I get it to work.
Comment on attachment 8797336 [details]
Remove mMappedFlows from TextRunData - bug 1290312

https://reviewboard.mozilla.org/r/82932/#review81804

Sorry for the delay in taking a look at this.

That's the only evident thing I've seen from looking at the patch (could be more, but I don't think so).

Clearing the review flag for now, let me know when you have an updated patch so I can take a look at it :)

Thanks for working on this once again!

::: layout/generic/nsTextFrame.cpp:2035
(Diff revision 1)
>    } else {
>      userData = CreateUserData(mMappedFlows.Length());
>      userDataToDestroy = userData;
>    }
>  
> +  TextRunMappedFlow* userMappedFlows = &dummyMappedFlow;

This is probably why your code is failing. You need to set `userMappedFlows` to `&dummyMappedFlow` only in the `if` condition.

Otherwise, you need to set it to `userData + 1`, which is the space you just created for the flows.

::: layout/generic/nsTextFrame.cpp:2437
(Diff revision 1)
>    } else {
>      userData = CreateUserData(mMappedFlows.Length());
>      userDataToDestroy = userData;
>    }
>  
> +  TextRunMappedFlow* userMappedFlows = &dummyMappedFlow;

And the same happens here.
Attachment #8797336 - Flags: review?(ecoal95)
Assignee: nobody → sumi29
@Emilio:
Thanks for pointing out my newbie-mistake and for bearing with me.
I updated the review-board with the changes, and the browser seems to run fine (no SEGVs or any other issues, and no new build warnings). RefTest suite passes with only 34 unexpected failures, but I am pretty sure I saw the same number without my changes too. 
I've tried to tidy up everything in this patch, but I might've missed something so please feel free to point it out.
Comment on attachment 8797336 [details]
Remove mMappedFlows from TextRunData - bug 1290312

https://reviewboard.mozilla.org/r/82932/#review81932

This looks nice! A few format nits and I'll hand the final review to Xidorn, who can formally approve this :)

::: layout/generic/nsTextFrame.cpp:520
(Diff revision 2)
>  }
>  
> +static TextRunMappedFlow*
> +GetMappedFlows(const gfxTextRun* aTextRun)
> +{
> +  MOZ_ASSERT(aTextRun->GetUserData());

Can you also MOZ_ASSERT that the textrun doesn't contain the `TEXT_IS_SIMPLE_FLOW` flag?

::: layout/generic/nsTextFrame.cpp:523
(Diff revision 2)
> +GetMappedFlows(const gfxTextRun* aTextRun)
> +{
> +  MOZ_ASSERT(aTextRun->GetUserData());
> +  if (aTextRun->GetFlags() & nsTextFrameUtils::TEXT_MIGHT_HAVE_GLYPH_CHANGES) {
> +    return reinterpret_cast<TextRunMappedFlow*>(
> +      (static_cast<ComplexTextRunUserData*>(aTextRun->GetUserData())) + 1);

nit: The parentheses around `static_cast` don't seem to be needed, here and below.

::: layout/generic/nsTextFrame.cpp:525
(Diff revision 2)
> +  MOZ_ASSERT(aTextRun->GetUserData());
> +  if (aTextRun->GetFlags() & nsTextFrameUtils::TEXT_MIGHT_HAVE_GLYPH_CHANGES) {
> +    return reinterpret_cast<TextRunMappedFlow*>(
> +      (static_cast<ComplexTextRunUserData*>(aTextRun->GetUserData())) + 1);
> +  }
> +  else {

nit: I think it's preferred to put both braces in the same line (`} else {`), but in any case, this else is just unneeded since it's preceded by a `return` statement.

::: layout/generic/nsTextFrame.cpp:2458
(Diff revision 2)
>      const nsTextFragment* frag = content->GetText();
>      int32_t contentStart = mappedFlow->mStartFrame->GetContentOffset();
>      int32_t contentEnd = mappedFlow->GetContentEnd();
>      int32_t contentLength = contentEnd - contentStart;
>  
> -    TextRunMappedFlow* newFlow = &userData->mMappedFlows[i];
> +    TextRunMappedFlow* newFlow = &(userMappedFlows[i]);

nit: These parentheses seem unnecessary.

::: layout/generic/nsTextFrame.cpp:2678
(Diff revision 2)
>    int32_t i = aUserData->mLastFlowIndex;
>    int32_t delta = 1;
>    int32_t sign = 1;
>    // Search starting at the current position and examine close-by
>    // positions first, moving further and further away as we go.
> -  while (i >= 0 && uint32_t(i) < aUserData->mMappedFlowCount) {
> +  while (i >= 0 && uint32_t(i) < aUserData->mMappedFlowCount) {  

nit: Trailing whitespace.

::: layout/generic/nsTextFrame.cpp:2679
(Diff revision 2)
>    int32_t delta = 1;
>    int32_t sign = 1;
>    // Search starting at the current position and examine close-by
>    // positions first, moving further and further away as we go.
> -  while (i >= 0 && uint32_t(i) < aUserData->mMappedFlowCount) {
> -    TextRunMappedFlow* flow = &aUserData->mMappedFlows[i];
> +  while (i >= 0 && uint32_t(i) < aUserData->mMappedFlowCount) {  
> +    TextRunMappedFlow* flow = &(userMappedFlows[i]);

nit: Unnecessary parentheses, here and below.
Attachment #8797336 - Flags: review?(ecoal95)
Comment on attachment 8797336 [details]
Remove mMappedFlows from TextRunData - bug 1290312

https://reviewboard.mozilla.org/r/82932/#review82006

Almost there! :)

::: layout/generic/nsTextFrame.cpp:520
(Diff revisions 2 - 3)
>  }
>  
>  static TextRunMappedFlow*
>  GetMappedFlows(const gfxTextRun* aTextRun)
>  {
> -  MOZ_ASSERT(aTextRun->GetUserData());
> +  MOZ_ASSERT(aTextRun->GetUserData() & !nsTextFrameUtils::TEXT_IS_SIMPLE_FLOW);

This assertion is incorrect, you should assert on one side that the text run has user data (that's `MOZ_ASSERT(aTextRun->GetUserData());`), and then _also_, on another line, that the textrun does not map a simple flow (`MOZ_ASSERT(!(aTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_SIMPLE_FLOW));`.
Attachment #8797336 - Flags: review?(ecoal95)
Comment on attachment 8797336 [details]
Remove mMappedFlows from TextRunData - bug 1290312

https://reviewboard.mozilla.org/r/82932/#review82006

> This assertion is incorrect, you should assert on one side that the text run has user data (that's `MOZ_ASSERT(aTextRun->GetUserData());`), and then _also_, on another line, that the textrun does not map a simple flow (`MOZ_ASSERT(!(aTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_SIMPLE_FLOW));`.

Wait, you mean two separate MOZ_ASSERTS on two separate lines?
(
MOZ_ASSERT(aTextRun->GetUserData());
MOZ_ASSERT(!(aTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_SIMPLE_FLOW));
)

Or just logically ANDing them both?
(
MOZ_ASSERT(aTextRun()->GetUserData() & !(aTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_SIMPLE_FLOW));
)

Anyway, the reason I did this is simply because we only asserted on GetUserData() everywhere else, so it seemed to me that asserting on GetUserData + SIMPLE_FLOW was the right thing to do. If we assert on GetFlags() here, then checking for GetFlags() in the if-condition seems unnecessary (which we seem to do everywhere else).
Comment on attachment 8797336 [details]
Remove mMappedFlows from TextRunData - bug 1290312

https://reviewboard.mozilla.org/r/82932/#review82006

> Wait, you mean two separate MOZ_ASSERTS on two separate lines?
> (
> MOZ_ASSERT(aTextRun->GetUserData());
> MOZ_ASSERT(!(aTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_SIMPLE_FLOW));
> )
> 
> Or just logically ANDing them both?
> (
> MOZ_ASSERT(aTextRun()->GetUserData() & !(aTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_SIMPLE_FLOW));
> )
> 
> Anyway, the reason I did this is simply because we only asserted on GetUserData() everywhere else, so it seemed to me that asserting on GetUserData + SIMPLE_FLOW was the right thing to do. If we assert on GetFlags() here, then checking for GetFlags() in the if-condition seems unnecessary (which we seem to do everywhere else).

Either way, I suggest you adding description to the assertion as the second argument, so that it is easier to know what you want to ensure (whether that assertion itself makes sense), and what your code are actually checking (whether the assertion code matches your description).
Comment on attachment 8797336 [details]
Remove mMappedFlows from TextRunData - bug 1290312

https://reviewboard.mozilla.org/r/82932/#review82144

This looks fine to me. Handing the final review to Xidorn since he can formally approve it.
Attachment #8797336 - Flags: review?(ecoal95) → review+
Attachment #8797336 - Flags: review?(xidorn+moz)
Comment on attachment 8797336 [details]
Remove mMappedFlows from TextRunData - bug 1290312

https://reviewboard.mozilla.org/r/82932/#review82006

> Either way, I suggest you adding description to the assertion as the second argument, so that it is easier to know what you want to ensure (whether that assertion itself makes sense), and what your code are actually checking (whether the assertion code matches your description).

> then checking for GetFlags() in the if-condition seems unnecessary

Why would it be? You're checking for two different flags.
Comment on attachment 8797336 [details]
Remove mMappedFlows from TextRunData - bug 1290312

https://reviewboard.mozilla.org/r/82932/#review82326

Thanks for the patch. It mostly looks good to me. Could you address the concerns below? I'd like to re-review when they are fixed.

::: layout/generic/nsTextFrame.cpp
(Diff revision 4)
>   * animatable glyphs.
>   *
>   * This way, we avoid allocating and constructing the extra nsTArray.
>   */
>  struct TextRunUserData {
> -  TextRunMappedFlow* mMappedFlows;

Having read this patch, I'm a bit concerned about the robustness of using so many pointer arithmetic.

Could you, instead of remove this field directly, wrap it in "#ifdef DEBUG" and "#endif" to make it available in debug build (same for the assignment in the two Create* functions), and use MOZ_ASSERT in GetMappedFlows() to verify that the result there matches the value it was assumed to be when created?

::: layout/generic/nsTextFrame.cpp:939
(Diff revision 4)
>        auto data = CreateComplexUserData(oldData->mMappedFlowCount);
> +      TextRunMappedFlow* dataMappedFlows = reinterpret_cast<TextRunMappedFlow*>(data + 1);;

Since you are doing pointer arithmetic, please expand the "auto" to the actual pointer type it is holding (should be "ComplexTextRunUserData*" here), so that it wouldn't be silently broken if someone decides to change the return type of CreateComplexUserData().

::: layout/generic/nsTextFrame.cpp:940
(Diff revision 4)
>    } else {
>      if (!(aTextRun->GetFlags() & nsTextFrameUtils::TEXT_MIGHT_HAVE_GLYPH_CHANGES)) {
>        auto oldData = static_cast<TextRunUserData*>(aTextRun->GetUserData());
> +      TextRunMappedFlow* oldMappedFlows = GetMappedFlows(aTextRun);
>        auto data = CreateComplexUserData(oldData->mMappedFlowCount);
> +      TextRunMappedFlow* dataMappedFlows = reinterpret_cast<TextRunMappedFlow*>(data + 1);;

nit: there are two ";"s at the end of the line. Please remove one.
Attachment #8797336 - Flags: review?(xidorn+moz)
Comment on attachment 8797336 [details]
Remove mMappedFlows from TextRunData - bug 1290312

https://reviewboard.mozilla.org/r/82932/#review82358

Looks good. r=me with the following nits addressed.

::: layout/generic/nsTextFrame.cpp:540
(Diff revision 5)
> +      static_cast<ComplexTextRunUserData*>(aTextRun->GetUserData()) + 1);
> +  } else {
> +    flows = reinterpret_cast<TextRunMappedFlow*>(
> +      static_cast<TextRunUserData*>(aTextRun->GetUserData()) + 1);
> +  }
> +#ifdef DEBUG

nit: MOZ_ASSERT doesn't need to be wrapped inside "#ifdef DEBUG". MOZ_ASSERT is completely removed in non-debug build.

::: layout/generic/nsTextFrame.cpp:541
(Diff revision 5)
> +  MOZ_ASSERT(static_cast<TextRunUserData*>(aTextRun->GetUserData())->mMappedFlows == flows,
> +            "GetMappedFlows should return the same pointer as mMappedFlows.");

nit: The assertion line exceeds 80 chars. I admit that it looks hard to break. Probably break the line after the last "->" or after "(" of static_cast.

And the description in second line should align to static_cast above it (one more whitespace there).

::: layout/generic/nsTextFrame.cpp:955
(Diff revision 5)
> +      ComplexTextRunUserData* data = CreateComplexUserData(oldData->mMappedFlowCount);
> +      TextRunMappedFlow* dataMappedFlows = reinterpret_cast<TextRunMappedFlow*>(data + 1);

nit: both lines exceed 80 characters. Please break after "=" here.

(I know there are lines exceed that limit around... but let's follow the coding style for new code.)

::: layout/generic/nsTextFrame.cpp:2686
(Diff revision 5)
>    }
>  }
>  
>  // Find the flow corresponding to aContent in aUserData
>  static inline TextRunMappedFlow*
> -FindFlowForContent(TextRunUserData* aUserData, nsIContent* aContent)
> +FindFlowForContent(TextRunUserData* aUserData, nsIContent* aContent, TextRunMappedFlow* userMappedFlows)

nit: please break this line into two lines. Probably put the last parameter to the second line, align to TestRunUserData (after the "(").

::: layout/generic/nsTextFrame.cpp:2769
(Diff revision 5)
>            auto userData = static_cast<TextRunUserData*>(oldTextRun->GetUserData());
> -          firstFrame = userData->mMappedFlows[0].mStartFrame;
> +          TextRunMappedFlow* userMappedFlows = GetMappedFlows(oldTextRun);
> +          firstFrame = userMappedFlows[0].mStartFrame;
>            if (MOZ_UNLIKELY(f != firstFrame)) {
>              TextRunMappedFlow* flow = FindFlowForContent(userData,
> -                                                         f->GetContent());
> +                                                         f->GetContent(), userMappedFlows);

nit: please wrap the line after the comma. Alternatively, you can wrap after the "=" or the "(" from the line before.

::: layout/generic/nsTextFrame.cpp:2854
(Diff revision 5)
>      return gfxSkipCharsIterator(textRun->GetSkipChars(), 0, mContentOffset);
>    }
>  
>    auto userData = static_cast<TextRunUserData*>(textRun->GetUserData());
> -  TextRunMappedFlow* flow = FindFlowForContent(userData, mContent);
> +  TextRunMappedFlow* userMappedFlows = GetMappedFlows(textRun);
> +  TextRunMappedFlow* flow = FindFlowForContent(userData, mContent, userMappedFlows);

nit: please break this line into two lines, probably after "=".
Attachment #8797336 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8797336 [details]
Remove mMappedFlows from TextRunData - bug 1290312

https://reviewboard.mozilla.org/r/82932/#review82358

> nit: The assertion line exceeds 80 chars. I admit that it looks hard to break. Probably break the line after the last "->" or after "(" of static_cast.
> 
> And the description in second line should align to static_cast above it (one more whitespace there).

The description still doesn't align after "(".

> nit: please break this line into two lines. Probably put the last parameter to the second line, align to TestRunUserData (after the "(").

This doesn't seem to be fixed.
Flags: needinfo?(prageetheranga)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2273369348da
Remove mMappedFlows from TextRunData - bug 1290312 r=emilio,xidorn
@Xidorn and @Emilio:
Thank you for your thorough feedback, and for bearing with me - much appreciated!
Thank you for fixing this!
https://hg.mozilla.org/mozilla-central/rev/2273369348da
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.