Opened 3 years ago

Closed 3 years ago

#5068 closed bug (wontfix)

Possible loss of sharing: big performance change

Reported by: simonpj Owned by:
Priority: normal Milestone: 7.4.1
Component: Compiler Version: 7.0.3
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty:
Test Case: Blocked By:
Blocking: Related Tickets:

Description

Bryan's blog post http://www.serpentine.com/blog/2011/03/18/a-little-care-and-feeding-can-go-a-long-way/ said that he got a massive performance change by changing some INLINE pragmas. GHC shouldn't ever duplicate work, as he implies: "causing the ziggurat tables to be regenerated over and over instead of precomputed just once".

Bryan writes: here's the changeset that shows exactly what I did.

https://bitbucket.org/bos/mwc-random/changeset/123ccdb62a3a

I'm using the vector package here, imported under the name I, to compute the "blocks" and "ratios" values, the two vectors I've marked as NOINLINE. These only need to be computed once, but the majority of the speed improvement I allude to in the blog post comes from marking them as NOINLINE (the other two INLINE annotations account for a few percent improvement, which is nice, but not a big deal).

Now, I'll confess that (for once) I didn't read the Core generated before and after to see if it was some *other* consequence of those pragmas that led to the speedups. It's possible that I misattributed the speedups to the recomputation of those vectors, when in fact something else was at work.

Bryan is going to get around to more detailed repro instructions; and perhaps glance at the core first to see if ‘ratios or ‘defaultSeed are getting inlined inside a lambda. This ticket is just to keep it on our radar.

Change History (7)

comment:1 follow-up: Changed 3 years ago by simonmar

If you say INLINE on a redex, GHC will happily do as you ask, won't it? I took a quick look at the diff that Bryan pointed to, and it looks like the problem is ratios being inlined in loop, which is a monadic computation and therefore probably a lambda.

comment:2 Changed 3 years ago by igloo

  • Milestone set to 7.4.1

comment:3 in reply to: ↑ 1 Changed 3 years ago by bos

Replying to simonmar:

If you say INLINE on a redex, GHC will happily do as you ask, won't it? I took a quick look at the diff that Bryan pointed to, and it looks like the problem is ratios being inlined in loop, which is a monadic computation and therefore probably a lambda.

I agree. Although I didn't think to check when I first wrote the code, when it occurred to me that there might be a performance problem, the computation of ratios was the very first thing I thought to look at. In other words, GHC's behaviour here was maybe a little unfortunate, but it wasn't unexpected (admittedly by a fairly sophisticated user).

I can submit a test case if that would be useful.

comment:4 Changed 3 years ago by simonpj

Oh, now I understand. You have something like this:

{-# INLINE bad #-}
bad n = let k = fib 200 
        in n + k

And GHC does exactly what you ask, replacing every call (bad e) with (let k = fib 200 in e + k). So the (fib 200) gets done once per call of bad. Fair enough. If you didn't have an INLINE pragma, GHC's full laziness transformation would lift out that constant expression, but INLINE (by design) does exactly what it says on the tin.

However I'm stil surprised you get such a massive slow-down. You'll get one call to (fib 200) per call of bad in the program text; ie only a fixed increment on execution time. For exmaple, if the call to bad was in a loop, the call to (fib 200) should then be lifted out of the loop:

loop 0 = 0
loop n = bad n + loop (n-1)

===>  after inlining

loop 0 = 0
loop n = let k = fib 200 in n + k + loop (n-1)

===>  full laziness

k = fib 200
loop 0 = 0
loop n = n + k + loop (n-1)

Moreover (and separately) I don't see how your changeset (linked above) would affect this behaviour.

So there is still a mystery here.

Simon

comment:5 Changed 3 years ago by simonmar

Indeed, now that I look at it again it does look suspicious, please ignore my earlier comment. We should look at the Core here and see what's going on.

comment:6 Changed 3 years ago by simonpj

  • Status changed from new to infoneeded

Nothing much is happening on this ticket. I think it's because we're a bit stuck here until/unless we can reproduce the problem. Looking at changesets isn't enough. Bryan: if you care about this, can you give us enough info to reproduce. If not, let's close it.

Simon

comment:7 Changed 3 years ago by bos

  • Resolution set to wontfix
  • Status changed from infoneeded to closed

I am not sure this is worth addressing. At the very least, I won't have any time to provide reproduction instructions for at least a couple of months.

Note: See TracTickets for help on using tickets.