Closed Bug 1251995 Opened 4 years ago Closed 4 years ago

Consider using struct to pass parameters for nsTextFrame::{Paint,Draw}*

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
58 bytes, text/x-review-board-request
jfkthame
: review+
Details
Many of those functions have a very long parameter list. It is hard to maintain, and potentially affect performance.

Although using structs may make it a bit verbose in some callsites, it should make things a lot clearer and easier to change.
This should also cover some of functions in gfxTextRun, so probably also related to Graphics:Text (or probably it is actually more Graphics than Layout?)
Although this makes some places more complicated, code should generally
be simpler and clearer. It could slightly improve performance on x64 for
functions with more than four parameters, since it makes more data be
passed via registers rather than the stack.

Review commit: https://reviewboard.mozilla.org/r/37521/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37521/
Attachment #8725638 - Flags: review?(jfkthame)
The motivation of this bug is that I probably need to add a new parameter to some of these functions for bug 1097499...

Using struct to pass parameters we get:
1. callsite verbosity, which is good because it would be eaiser to know what value is for what parameter, and we don't need to take care of the order; and
2. simpler function signature, which is also good as it is not useful to repeat the parameter list in declaration, and fewer function parameters may decrease the overhead of function call and give compilers more opportunity to optimize.

One disadvantage is that we lose the ability to pass reference, which makes it impossible to enforce a must-provide pattern in compile time for some of the parameters. This is probably unsolvable within the current C++.
I haven't actually read the patches yet, but this seems like good cleanup for some code that's grown pretty crufty over the years -- thanks!

One request: please kick off a try run that includes talos (probably tp5o is the most interesting) tests for the various platforms (better include PGO builds, too), so we can see if there's any visible impact on overall performance. Some of these codepaths can be pretty hot, and it's not always obvious what the perf benefits or costs will be.
Talos: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f401fd595a3

Although it is possible to trigger PGO build on try server, its result may not be comparable, since try server cannot distinguish a PGO build from normal opt builds.
Errrr, I didn't know I need to retrigger talos for my base revision as well... then let's wait...
Compare: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=1a94e5cfe9e7&newProject=try&newRevision=5f401fd595a3&framework=1&showOnlyImportant=0

It doesn't look like there are significant changes. The two items marked red and orange seem to be noisy themselves. Also I wonder whether my part 4 could have made the result less useful. I probably should separate that one to another bug and investigate it there. I did part 4 for part 5, but it seems it is not that necessary...
Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37521/diff/1-2/
Comment on attachment 8725639 [details]
MozReview Request: Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37523/diff/1-2/
Comment on attachment 8725640 [details]
MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37525/diff/1-2/
Attachment #8725642 - Attachment description: MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame. → MozReview Request: Bug 1251995 part 4 - Unify units of dirty rect used for painting text frame.
Comment on attachment 8725642 [details]
MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37529/diff/1-2/
Comment on attachment 8725643 [details]
MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37531/diff/1-2/
Attachment #8725643 - Attachment description: MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions. → MozReview Request: Bug 1251995 part 5 - Use struct to pass params for nsTextFrame::PaintText* functions.
Comment on attachment 8725644 [details]
MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37533/diff/1-2/
Attachment #8725644 - Attachment description: MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions. → MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.
Attachment #8725641 - Attachment is obsolete: true
Attachment #8725641 - Flags: review?(mstange)
Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

https://reviewboard.mozilla.org/r/37521/#review34363

I haven't read all the way through this yet (though I expect it's fine), but I do have a global change to request so I'll go ahead and post this. I added a note inline the first time it came up, but this applies throughout... wherever you've currently got something like:

    SomeMethod(param1, {foo, bar}, moreParams, ...);

to pass a Range, I'd prefer to see

    SomeMethod(param1, Range(foo, bar), moreParams, ...);

as I think that will help readability throughout this code.

::: dom/canvas/CanvasRenderingContext2D.cpp:3503
(Diff revision 2)
> -                                                               mTextRun->GetLength(),
> +        {0, mTextRun->GetLength()},

I think it'd make callsites more readable if you provide a constructor for `gfxTextRun::Range`, and then use it in places like this instead of the anonymous braces.

::: gfx/src/nsFontMetrics.cpp:362
(Diff revision 2)
> +    gfxTextRun::Range range{0, aLength};

And for consistency I'd then go for `range(0, aLength)` here, too, although a declaration like this isn't as cryptic as the typical function callsites.

::: gfx/thebes/gfxTextRun.cpp:977
(Diff revision 2)
> -        *aMetrics = MeasureText(aStart, charsFit, aBoundingBoxType,
> +        *aMetrics = MeasureText({aStart, aStart + charsFit}, aBoundingBoxType,

A local `uint32_t end = aStart + charsFit;` could be used here (three times) to improve readability.
Attachment #8725638 - Flags: review?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #22)
> Comment on attachment 8725638 [details]
> MozReview Request: Bug 1251995 part 1 - Add gfxTextRun::Range to replace
> parameter pairs like (offset, length) and (start, end).
> 
> https://reviewboard.mozilla.org/r/37521/#review34363
> 
> I haven't read all the way through this yet (though I expect it's fine), but
> I do have a global change to request so I'll go ahead and post this. I added
> a note inline the first time it came up, but this applies throughout...
> wherever you've currently got something like:
> 
>     SomeMethod(param1, {foo, bar}, moreParams, ...);
> 
> to pass a Range, I'd prefer to see
> 
>     SomeMethod(param1, Range(foo, bar), moreParams, ...);
> 
> as I think that will help readability throughout this code.

Hmmm, many places would actually need gfxTextRun::Range() rather than just Range(), which would make code much longer and harder to arrange.

The main idea of introducing gfxTextRun::Range is to make start/end (and offset/length) an integrated pair. I think using brace should enough make it clear that it is an integrated pair, which should not be more cryptic, and actually should have been much better than the current form for callsites. It seems to me having their type listed there isn't that important.

Do you agree with this argument?

> ::: dom/canvas/CanvasRenderingContext2D.cpp:3503
> (Diff revision 2)
> > -                                                               mTextRun->GetLength(),
> > +        {0, mTextRun->GetLength()},
> 
> I think it'd make callsites more readable if you provide a constructor for
> `gfxTextRun::Range`, and then use it in places like this instead of the
> anonymous braces.

I agree that adding a constructor for whole range of a text run makes sense.
Flags: needinfo?(jfkthame)
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #23)
> (In reply to Jonathan Kew (:jfkthame) from comment #22)
> > ::: dom/canvas/CanvasRenderingContext2D.cpp:3503
> > (Diff revision 2)
> > > -                                                               mTextRun->GetLength(),
> > > +        {0, mTextRun->GetLength()},
> > 
> > I think it'd make callsites more readable if you provide a constructor for
> > `gfxTextRun::Range`, and then use it in places like this instead of the
> > anonymous braces.
> 
> I agree that adding a constructor for whole range of a text run makes sense.

Hmmm... 
> gfxTextRun::Range(mTextRun)
is actually longer than
> {0, mTextRun->GetLength()}
which makes me wonder whether I really should add it...
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #23)
> (In reply to Jonathan Kew (:jfkthame) from comment #22)
> Hmmm, many places would actually need gfxTextRun::Range() rather than just
> Range(), which would make code much longer and harder to arrange.

Other classes that call these methods, and need to construct a gfxTextRun::Range to pass, can just do their own

    typedef gfxTextRun::Range Range;

to avoid needing to use the long form at all the callsites.

I'd still prefer to see  Range(a, b)  rather than bare  {a, b}  in all those parameter lists. If we're going to touch all these lines of code, let's go for the most-readable option. IMO that wins over being a few characters shorter.


(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #24)

> Hmmm... 
> > gfxTextRun::Range(mTextRun)
> is actually longer than
> > {0, mTextRun->GetLength()}
> which makes me wonder whether I really should add it...

It becomes just

    Range(mTextRun)

if class (or method) using it has done its own typedef, as above.
Flags: needinfo?(jfkthame)
Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37521/diff/2-3/
Attachment #8725638 - Attachment description: MozReview Request: Bug 1251995 part 1 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end). → MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).
Attachment #8725638 - Flags: review?(jfkthame)
Comment on attachment 8725639 [details]
MozReview Request: Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37523/diff/2-3/
Attachment #8725639 - Attachment description: MozReview Request: Bug 1251995 part 2 - Use struct to pass params for gfxTextRun::Draw. → MozReview Request: Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw.
Comment on attachment 8725640 [details]
MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37525/diff/2-3/
Attachment #8725640 - Attachment description: MozReview Request: Bug 1251995 part 3 - Use struct to pass params for nsTextFrame::DrawText* functions. → MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.
Comment on attachment 8725642 [details]
MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37529/diff/2-3/
Attachment #8725642 - Attachment description: MozReview Request: Bug 1251995 part 4 - Unify units of dirty rect used for painting text frame. → MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame.
Comment on attachment 8725643 [details]
MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37531/diff/2-3/
Attachment #8725643 - Attachment description: MozReview Request: Bug 1251995 part 5 - Use struct to pass params for nsTextFrame::PaintText* functions. → MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions.
Comment on attachment 8725644 [details]
MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37533/diff/2-3/
Attachment #8725644 - Attachment description: MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::Paint*Shadow functions. → MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.
Comment on attachment 8726695 [details]
MozReview Request: Bug 1251995 part 1 - Add helper functions to simplify code.

https://reviewboard.mozilla.org/r/38191/#review34713

Yes, this is some nice cleanup, thanks.
Attachment #8726695 - Flags: review?(jfkthame) → review+
Attachment #8725638 - Flags: review?(jfkthame) → review+
Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

https://reviewboard.mozilla.org/r/37521/#review34719
Comment on attachment 8725639 [details]
MozReview Request: Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw.

https://reviewboard.mozilla.org/r/37523/#review34913
Attachment #8725639 - Flags: review?(jfkthame) → review+
Comment on attachment 8725640 [details]
MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

https://reviewboard.mozilla.org/r/37525/#review34915

::: layout/generic/nsTextFrame.h:404
(Diff revision 3)
> +  };

For the fields that don't have a default, could we set them in the constructor, so that it's guaranteed that they are never left uninitialized?

::: layout/generic/nsTextFrame.h:415
(Diff revision 3)
> +  };

Likewise, can we simply set framePt, dirtyRect, textStyle and clipEdges with the constructor?
Attachment #8725640 - Flags: review?(jfkthame)
Attachment #8725642 - Flags: review?(jfkthame) → review+
Comment on attachment 8725642 [details]
MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame.

https://reviewboard.mozilla.org/r/37529/#review34917
(In reply to Jonathan Kew (:jfkthame) from comment #36)
> Comment on attachment 8725640 [details]
> MozReview Request: Bug 1251995 part 4 - Use struct to pass params for
> nsTextFrame::DrawText* functions.
> 
> https://reviewboard.mozilla.org/r/37525/#review34915
> 
> ::: layout/generic/nsTextFrame.h:404
> (Diff revision 3)
> > +  };
> 
> For the fields that don't have a default, could we set them in the
> constructor, so that it's guaranteed that they are never left uninitialized?

I tend not to do so as it ruins callsite verbosity and bloat single function param list again... Actually I'm wondering whether I should also pull |context| out from the constructor.

Using uninitialized field here should cause runtime bustage on ASAN build, since the code paths here should have already been well covered in our tests. So I'm not too concerned about this. Also, hoping ASAN to catch such mistakes is the reason I didn't give every field a default value.
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #38)
> (In reply to Jonathan Kew (:jfkthame) from comment #36)
> Using uninitialized field here should cause runtime bustage on ASAN build,
> since the code paths here should have already been well covered in our
> tests. So I'm not too concerned about this. Also, hoping ASAN to catch such
> mistakes is the reason I didn't give every field a default value.

Will ASAN catch reading an uninitialized field (e.g. a bool like |drawSoftHyphen|)? Valgrind would, but we don't routinely run our test suite under valgrind, afaik...
Hmmm... It seems you're right that ASAN does not catch that... But I still don't want to bloat the param list of a single function...

We may need some new technique here...
Probably we should just initialize all things by default in the structure, and add assertions in callees if necessary.
Comment on attachment 8726695 [details]
MozReview Request: Bug 1251995 part 1 - Add helper functions to simplify code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38191/diff/1-2/
Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37521/diff/3-4/
Comment on attachment 8725639 [details]
MozReview Request: Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37523/diff/3-4/
Attachment #8725640 - Flags: review?(jfkthame)
Comment on attachment 8725640 [details]
MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37525/diff/3-4/
Comment on attachment 8725642 [details]
MozReview Request: Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37529/diff/3-4/
Comment on attachment 8725643 [details]
MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37531/diff/3-4/
Comment on attachment 8725644 [details]
MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37533/diff/3-4/
https://reviewboard.mozilla.org/r/37525/#review34915

> For the fields that don't have a default, could we set them in the constructor, so that it's guaranteed that they are never left uninitialized?

In the new revision, I give all of them a default value, so that they would never be left uninitialized. We could observe determinated behavior if they are not initialized correctly.

> Likewise, can we simply set framePt, dirtyRect, textStyle and clipEdges with the constructor?

I'm not too concerned about framePt and dirtyRect. gfxPoint and gfxRect have default constructor which initializes their fields to zero. And I've also changed part 2 to make gfxTextRun::Range also do that. The pointer fields all have a default nullptr value now, so we would see nullptr dereference crash if they are not initialized properly.
Attachment #8725640 - Flags: review?(jfkthame) → review+
Comment on attachment 8725640 [details]
MozReview Request: Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions.

https://reviewboard.mozilla.org/r/37525/#review34989

OK, I like this better now - thanks. :)
Attachment #8725643 - Flags: review?(jfkthame) → review+
Comment on attachment 8725643 [details]
MozReview Request: Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions.

https://reviewboard.mozilla.org/r/37531/#review34991
Attachment #8725644 - Flags: review?(jfkthame) → review+
Comment on attachment 8725644 [details]
MozReview Request: Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions.

https://reviewboard.mozilla.org/r/37533/#review34999
Assignee: nobody → quanxunzhen
backed out for test failures in reftests like https://treeherder.mozilla.org/logviewer.html#?job_id=23143811&repo=mozilla-inbound
Flags: needinfo?(quanxunzhen)
Comment on attachment 8725638 [details]
MozReview Request: Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end).

https://reviewboard.mozilla.org/r/37521/#review35017

::: layout/generic/nsTextFrame.cpp:6624
(Diff revision 4)
> -      gfxFloat hyphenBaselineX = aTextBaselinePt.x + mTextRun->GetDirection() * aAdvanceWidth -
> +      gfxFloat hyphenBaselineX =

This looks like it's probably the cause of those reftest failures. We still need aAdvanceWidth incorporated into the hyphen's position...
Attachment #8725638 - Flags: review+
https://reviewboard.mozilla.org/r/37521/#review35017

> This looks like it's probably the cause of those reftest failures. We still need aAdvanceWidth incorporated into the hyphen's position...

Hmmm... this is probably because of merge or even automerge...
There are several other reftest failures on OS X and Android... Probably I cannot fix them today then :(
It seems the failures on OS X and Android are from part 5, which unifies the unit of dirty rect. It seems the units in text frame are pretty... complicated...
Blocks: 1254375
https://hg.mozilla.org/integration/mozilla-inbound/rev/51f3d3a22d4251b8f4c0e70cb5161d75a46dfea2
Bug 1251995 part 1 - Add helper functions to simplify code. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d462b564421dc46a14c0d2b538413cd623bd37e
Bug 1251995 part 2 - Add gfxTextRun::Range to replace parameter pairs like (offset, length) and (start, end). r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dcad93e1045080c6f2476e296e8f52c1549cf7d
Bug 1251995 part 3 - Use struct to pass params for gfxTextRun::Draw. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/788178d64bf45b07070fffc96ea9d1640363f7a3
Bug 1251995 part 4 - Use struct to pass params for nsTextFrame::DrawText* functions. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5a5660267aeb4254d0dd9cc15ff2d1980790e4e
Bug 1251995 part 5 - Unify units of dirty rect used for painting text frame. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/366047809301992f99d3160b099063d16adee2a7
Bug 1251995 part 6 - Use struct to pass params for nsTextFrame::PaintText* functions. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffce50b96ca9482490e590735e694991e9085b11
Bug 1251995 part 7 - Use struct to pass params for nsTextFrame::Paint*Shadow functions. r=jfkthame
Flags: needinfo?(quanxunzhen)
Blocks: 1097499
You need to log in before you can comment on or make changes to this bug.