Opened 2 years ago

Closed 4 months ago

#8990 closed bug (fixed)

Performance tests behave differently depending on presence of .hi file (even with -fforce-recomp)

Reported by: ezyang Owned by: thomie
Priority: normal Milestone: 8.2.1
Component: Test Suite Version: 7.9
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D1187
Wiki Page:

Description

Example:

[ezyang@hs01 compiler]$ cat T6048.comp.stats  
/home/hs01/ezyang/ghc-compalloc/inplace/lib/bin/ghc-stage2 -B/home/hs01/ezyang/ghc-compalloc/inplace/lib -fforce-recomp -dno-debug-output -no-user-package-db -rtsopts -fno-ghci-history -c T6048.hs -O -fasm +RTS -V0 -tT6048.comp.stats --machine-readable 
 [("bytes allocated", "119871672")
 ,("num_GCs", "100")
 ,("average_bytes_used", "4388904")
 ,("max_bytes_used", "12539576")
 ,("num_byte_usage_samples", "7")
 ,("peak_megabytes_allocated", "27")
 ,("init_cpu_seconds", "0.00")
 ,("init_wall_seconds", "0.00")
 ,("mutator_cpu_seconds", "0.09")
 ,("mutator_wall_seconds", "0.10")
 ,("GC_cpu_seconds", "0.15")
 ,("GC_wall_seconds", "0.15")
 ]
[ezyang@hs01 compiler]$ rm T6048.hi
[ezyang@hs01 compiler]$ '/home/hs01/ezyang/ghc-compalloc/inplace/bin/ghc-stage2' -fforce-recomp -dno-debug-output -no-user-package-db -rtsopts -fno-ghci-history -c T6048.hs -O -fasm  +RTS -V0 -tT6048.comp.stats --machine-readable -RTS
[ezyang@hs01 compiler]$ cat T6048.comp.stats
/home/hs01/ezyang/ghc-compalloc/inplace/lib/bin/ghc-stage2 -B/home/hs01/ezyang/ghc-compalloc/inplace/lib -fforce-recomp -dno-debug-output -no-user-package-db -rtsopts -fno-ghci-history -c T6048.hs -O -fasm +RTS -V0 -tT6048.comp.stats --machine-readable 
 [("bytes allocated", "121022912")
 ,("num_GCs", "100")
 ,("average_bytes_used", "4022069")
 ,("max_bytes_used", "11224944")
 ,("num_byte_usage_samples", "7")
 ,("peak_megabytes_allocated", "25")
 ,("init_cpu_seconds", "0.00")
 ,("init_wall_seconds", "0.00")
 ,("mutator_cpu_seconds", "0.09")
 ,("mutator_wall_seconds", "0.11")
 ,("GC_cpu_seconds", "0.15")
 ,("GC_wall_seconds", "0.14")
 ]

It's a slight but present difference: I noticed because one of the perf tests was failing on an initial run of the test suite, but passing when I re-ran it.

There are two possible bugs here. One is that our implementation of -fforce-recomp is buggy and we shouldn't be reading in the hi file at all if -fforce-recomp is on; the other is that we should always clean up interface files before running one of these perf tests.

Change History (11)

comment:1 Changed 2 years ago by simonpj

Yes, I expect that we are reading the old interface file even if -fforce-recomp is on. It's a bit of a corner case, but I'd be happy to see it fixed if someone would like to construct a patch to stop that happening, and it's not too painful in code terms.

Simon

comment:2 Changed 18 months ago by thomie

  • Keywords newcomer added

Look in the compiler/main directory. Grep for ForceRecomp. Try to figure out what's going on (will take you a while).

Tickets #4114 and #2258 are related.

Last edited 16 months ago by thomie (previous) (diff)

comment:3 Changed 13 months ago by przembot

  • Owner set to przembot

comment:4 Changed 13 months ago by ezyang

I found another case recently where this matters: whether or not an interface file is loaded can affect the assignment of uniques, which in turn can cause rules to fire in a different order. (in my case, it was T7837 with the patchset I've been developing for #10871).

comment:5 Changed 12 months ago by thomie

  • Differential Rev(s) set to Phab:D1267
  • Milestone set to 8.0.1
  • Status changed from new to patch

comment:6 Changed 11 months ago by ezyang

  • Differential Rev(s) Phab:D1267 deleted
  • Owner przembot deleted
  • Status changed from patch to new

On the Phabricator CR, Simon Marlow pointed out that pretending the interface file doesn't exist when forcing recompilation can result in gratuitous changes to the timestamp on the interface file. So we can't take this approach; let's just remove hi files before perf tests.

comment:7 follow-up: Changed 8 months ago by thomie

  • Milestone 8.0.1 deleted

comment:8 in reply to: ↑ 7 Changed 5 months ago by akst

Hi, I was directed here from newcomers page. Just to clarify ForceRecomp is a constructor of GeneralFlag, and Frorcerecomp is actually Opt_ ForceRecomp now? Or should that be disregarded as the most recent suggestion on the issue is too "just remove hi files before perf tests"?

comment:9 Changed 5 months ago by thomie

You can ignore the hint about Opt_ ForceRecomp.

One possible solution:

  • Add -no-keep-hi-files (see #4114) for performance tests to the function get_compiler_flags in the file testsuite/driver/testlib.py.

Edit: but only when the compiler version >= 8.1. It should be possible to run the testsuite with an older compiler.

Last edited 5 months ago by thomie (previous) (diff)

comment:10 Changed 5 months ago by thomie

  • Differential Rev(s) set to Phab:D1187
  • Keywords newcomer removed
  • Owner set to thomie

I think Phab:D1187 will fix this one as well.

comment:11 Changed 4 months ago by thomie

  • Milestone set to 8.2.1
  • Resolution set to fixed
  • Status changed from new to closed

Fixed by running each test in a fresh /tmp directory (#11980).

Note: See TracTickets for help on using tickets.