Add macro versions of GetMainComponent/GetCrossComponent, to avoid unnecessarily evaluating expensive function calls

RESOLVED FIXED in mozilla27

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla27
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
FlexboxAxisTracker has several "GetMainComponent" utility methods that takes e.g. a nsSize and returns the width or height, depending on whether the main axis is horizontal or vertical.

Right now, I construct a nsSize on the fly to pass to that method, but that's somewhat wasteful when we're going to throw away half of the nsSize anyway.  It's even more wasteful if the width and/or height that I pass in are an expensive expression -- in that case, we'd like to make sure we only bother computing the argument that actually gets used.

(I've got at least one patch for flexbox pagination where this is important, where right now I'd be passing in
  nsSize(aReflowState.GetComputedWidth,
         GetEffectiveComputedHeight(aReflowState))
and the latter is expensive to compute, so it'd be nice to make sure we only compute it if we need it.)

We can achieve this using a macro. Patch coming up.
(Assignee)

Comment 1

4 years ago
Created attachment 807448 [details] [diff] [review]
part 1 v1: Add the macros (and remove unused utility methods)

Here's the first part, which:
 - Adds the macros
 - Tweaks the existing utility methods to use the macros, to share code / minimize the number of points of failure.
 - Removes two utility methods that turned out to be unused (which take nsHTMLReflowMetrics)
Attachment #807448 - Flags: review?(matspal)
(Assignee)

Updated

4 years ago
Attachment #807448 - Attachment description: 918519-part1.patch → part 1 v1
(Assignee)

Updated

4 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 2

4 years ago
Created attachment 807450 [details] [diff] [review]
part 2 v1: Use the macros to avoid unnecessary nsSize() instantiations
(Assignee)

Updated

4 years ago
Attachment #807448 - Attachment description: part 1 v1 → part 1 v1: Add the macros (and remove unused utility methods)
(Assignee)

Comment 3

4 years ago
Comment on attachment 807450 [details] [diff] [review]
part 2 v1: Use the macros to avoid unnecessary nsSize() instantiations

This second patch takes all of the Get[Main|Cross]Component() method-calls that perform an on-the-fly nsSize() instantiation, and replaces them with the macro.

This avoids an unnecessary nsSize instantiation, removes a tiny amount of boilerplate, and allows for either arg to be expensive-to-compute while still letting the function-call be fast when that arg is discarded.

(Aside: this actually leaves us with no callers of the nsSize-accepting Get[Main|Cross]Component() utility methods, but I think I'd still like to keep that pair of methods, since there's a good chance we'll need them at some point in the future.)
Attachment #807450 - Attachment description: part 2 v1 → part 2 v1: Use the macros to avoid unnecessary nsSize() instantiations
Attachment #807450 - Flags: review?(matspal)
(Assignee)

Updated

4 years ago
Blocks: 811024
Comment on attachment 807448 [details] [diff] [review]
part 1 v1: Add the macros (and remove unused utility methods)

>+#define GET_CROSS_COMPONENT(axisTracker_, width_, height_)  \
>+  IsAxisHorizontal(axisTracker_.GetCrossAxis()) ? width_ : height_

Please wrap each parameter use in ().

Remove those at the call sites.
Attachment #807448 - Flags: review?(matspal) → review+
Comment on attachment 807450 [details] [diff] [review]
part 2 v1: Use the macros to avoid unnecessary nsSize() instantiations

nsHTMLReflowState::ComputedWidth()/ComputedHeight() are inline accessor
methods that just return the corresponding field, so I don't see the
need to use the macro for performance reasons in any of these places.

I think I'd prefer to leave them as is, but I leave the choice to you.
Attachment #807450 - Flags: review?(matspal) → review+
(Assignee)

Comment 6

4 years ago
(In reply to Mats Palmgren (:mats) from comment #4)
> Please wrap each parameter use in ().
> 
> Remove those at the call sites.

Ah, right. :) Thanks! (I guess that's a giveaway that I haven't written many macros... my undergrad CS program discouraged them heavily)

(In reply to Mats Palmgren (:mats) from comment #5)
> nsHTMLReflowState::ComputedWidth()/ComputedHeight() are inline accessor
> methods that just return the corresponding field so I don't see the
> need to use the macro for performance reasons in any of these places.

Yeah -- that's wasn't for performance, but rather to remove unnecessary boilerplate -- the on-the-fly "nsSize()", which might have a (teensy) perf cost, and which doesn't add any value in any case.

It's also for consistency, since I'd at least need to switch one of them to using the macro bug 811024 (for a s/ComputedHeight()/GetEffectiveComputedHeight()/ change), and I might as well switch all of them so there's less of a superficially-arbitrary-looking difference in which incantation I perform.

Previously, the rule was...
 (a) if you have a nsIntSize or a nsSize, pass it directly to axisTracker.GetMainComponent()
 (b) if you just have two nscoord/integer values (or two nscoord/integer-valued-expressions), wrap them in a ns[Int]Size and then pass that to axisTracker.GetMainComponent().

...and I'm changing (b) to be "just call the macro" (which wasn't previously an option, but now is).  This allows for the nscoord-valued-expressions to be expensive, even though the ones changed in this patch aren't currently expensive.

So: I'm going to take you up on the option of leaving part 2 as-is ;) and land. Thanks for the review!
(Assignee)

Comment 7

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4821c95e7116
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc4d137f1cce
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/4821c95e7116
https://hg.mozilla.org/mozilla-central/rev/dc4d137f1cce
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.