Move trivial MBasicBlock accessors to the header file

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Posted patch PatchSplinter Review
I was profiling a single speedometer test and MBasicBlock::getSlot is one of the non-inlined functions that's called most frequently (> 225,000 calls!).

Turns out MBasicBlock has a lot of trivial methods that are very hot, but because they're defined in the .cpp file the compiler can't inline them.
Attachment #8892953 - Flags: review?(sstangl)
(In reply to Jan de Mooij [:jandem] from comment #0)
> Turns out MBasicBlock has a lot of trivial methods that are very hot, but
> because they're defined in the .cpp file the compiler can't inline them.

Can you add a comment about the measured perf impact?  I find we over-inline a lot of code in headers for no reason, but sometimes it does matter.  It would be nice to be able to tell the difference when we have a measured perf win due to inlining.
(Assignee)

Comment 2

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #1)
> Can you add a comment about the measured perf impact?  I find we over-inline
> a lot of code in headers for no reason, but sometimes it does matter.  It
> would be nice to be able to tell the difference when we have a measured perf
> win due to inlining.

I haven't measured that yet, but MBasicBlock::getSlot for instance is super hot (> 200,000 calls on a single test) and is just a single load in opt builds. It's definitely faster to just inline this than paying the call overhead.
I just meant a comment like:

 // NOTE: These methods are hot.  Do not un-inline them without measuring the impact.
Comment on attachment 8892953 [details] [diff] [review]
Patch

Looks fine to me.

The inclusion of MBasicBlock::add() is somewhat arbitrary, since that function is a bit less simple -- by what standard is addPhi() then not also in the header?
Attachment #8892953 - Flags: review?(sstangl) → review+
(Assignee)

Comment 5

2 years ago
(In reply to Ben Kelly [:bkelly] from comment #3)
>  // NOTE: These methods are hot.  Do not un-inline them without measuring
> the impact.

OK I can add something.

(In reply to Sean Stangl [:sstangl] from comment #4)
> The inclusion of MBasicBlock::add() is somewhat arbitrary, since that
> function is a bit less simple -- by what standard is addPhi() then not also
> in the header?

Yeah, MBasicBlock::add() is a bit less obvious, but I included it because it's pretty hot: my single-speedometer-subtest profile had > 59,000 calls to it. I just checked and addPhi is called 529 times so it's less hot and it's probably okay to keep it out-of-line.

Comment 6

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71228e37d789
Define some trivial MBasicBlock methods in the header file so the compiler can inline them. r=sstangl

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71228e37d789
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 8

2 years ago
https://hg.mozilla.org/projects/date/rev/71228e37d789ff5f8846099bb71cafa426d263e9
Bug 1386700 - Define some trivial MBasicBlock methods in the header file so the compiler can inline them. r=sstangl
You need to log in before you can comment on or make changes to this bug.