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.
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)
Attachment #807448 - Attachment description: 918519-part1.patch → part 1 v1
Created attachment 807450 [details] [diff] [review] part 2 v1: Use the macros to avoid unnecessary nsSize() instantiations
Attachment #807448 - Attachment description: part 1 v1 → part 1 v1: Add the macros (and remove unused utility methods)
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.)
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+
(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!
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.