1386700 - Move trivial MBasicBlock accessors to the header file

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

(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.

Comment 2

[Jan de Mooij [:jandem]]

(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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I just meant a comment like:

 // NOTE: These methods are hot.  Do not un-inline them without measuring the impact.

Comment 4

[Sean Stangl [:sstangl]]

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?

Comment 5

[Jan de Mooij [:jandem]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/71228e37d789

Comment 8

[Aki Sasaki (not active)]

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