Opened 4 years ago

Closed 17 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 4 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 3 years 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 2 years ago by thomie (previous) (diff)

comment:3 Changed 2 years ago by przembot

Owner: set to przembot

comment:4 Changed 2 years 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 2 years ago by thomie

Differential Rev(s): Phab:D1267
Milestone: 8.0.1
Status: newpatch

comment:6 Changed 2 years ago by ezyang

Differential Rev(s): Phab:D1267
Owner: przembot deleted
Status: patchnew

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 Changed 21 months ago by thomie

Milestone: 8.0.1

comment:8 in reply to:  7 Changed 18 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 18 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 18 months ago by thomie (previous) (diff)

comment:10 Changed 18 months ago by thomie

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

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

comment:11 Changed 17 months ago by thomie

Milestone: 8.2.1
Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.