Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5715 closed bug (invalid)

Inliner fails to inline a function, causing 20x slowdown

Reported by: bos Owned by:
Priority: normal Milestone: 7.4.2
Component: Compiler Version: 7.2.1
Keywords: Cc: pho@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Alexey filed a bug against the mwc-random package recently, indicating a 20x to 40x slowdown on a function named uniformRange - you can see its source here.

In the original definition, there was an INLINE pragma, but Alexey noticed that it wasn't firing and so performance was predictably terrible. He added the SPECIALIZE pragmas that now follow the body of the function.

I looked at -ddump-simpl output with the SPECIALIZE pragmas removed, and sure enough there are no inlining annotations on the function.

The whole purpose of uniformRange is to be used in instance declarations such as the following:

instance Variate Int8 where
    uniform  = uniform1 fromIntegral
    uniformR = uniformRange
    {-# INLINE uniform  #-}
    {-# INLINE uniformR #-}

I have a suspicion that what's going on is that GHC's inliner is declining to do anything because some call site or other (or perhaps several?) isn't fully saturated.

The behaviour of the new inliner is subtle to understand at times - it's not at all obvious when I should rewrite an instance like this, just to satisfy it:

instance Variate Int8 where
    uniform  = uniform1 fromIntegral
    uniformR inliner sacrifice = uniformRange inliner sacrifice
    {-# INLINE uniform  #-}
    {-# INLINE uniformR #-}

Saturating as above turned out to be the solution to the performance problem. I've been able to remove the SPECIALIZE pragmas. However, I'm still worried.

It would be helpful if GHC had a mode that dumped out when (and why) inlinings do *not* take place on functions that have been annotated with INLINE, because I'm surely not the only person who gets caught by this.

Also, aesthetically I find that saturating an application as above makes for tricky-to-read "why are those arguments there?" code, sort of the inliner's version of the dreaded monomorphism restriction: a lexical tic that's tremendously important, but for reasons that most readers will not know about.

Change History (6)

comment:1 Changed 8 years ago by simonpj

difficulty: Unknown

The behaviour of INLINE should be simple and predictable. If you write

f x y = <rhs>
{-# INLINE f #-}

then f will be inlined into any application (f e1 e2). But not into partial applications.

Consider the example you give:

instance Variate Int8 where
    uniform  = uniform1 fromIntegral
    uniformR = uniformRange
    {-# INLINE uniform  #-}
    {-# INLINE uniformR #-}
  • `uniformRange will not be inlined here. (Doing so would do no good.)
  • Every use of uniformR (at type Int8) should replaced by uniformRange (since uniformR has an INLINE pragma and has no args).
  • If that call to uniformR had two arguments, then uniformRange should in turn inline.

I see no reason to write

instance Variate Int8 where
    uniform  = uniform1 fromIntegral
    uniformR inliner sacrifice = uniformRange inliner sacrifice
    {-# INLINE uniform  #-}
    {-# INLINE uniformR #-}

Maybe that is a bug.

If you believe that HEAD (or even 7.2) is showing odd behaviour, could you make a standalone test case? Then I will certainly look at it.

Thanks

Simon

comment:2 Changed 8 years ago by bos

I'll see if I can repro this on HEAD.

If I don't write that peculiar definitional form of uniformR, then uniformR (and presumably uniformRange) is never inlined, so the difference between the two instances in the report above is indeed significant.

I'll see if I can find a smaller repro with HEAD.

comment:3 Changed 8 years ago by simonpj

OK, that might be a bug. Please do repro if you can

Thanks.

Simon

comment:4 Changed 8 years ago by PHO

Cc: pho@… added

comment:5 Changed 8 years ago by igloo

Milestone: 7.4.2
Status: newinfoneeded

comment:6 Changed 8 years ago by igloo

Resolution: invalid
Status: infoneededclosed

No testcase, so closing.

Note: See TracTickets for help on using tickets.