[GHC] #8308: Resurrect ticky code for counting constructor arity
#8308: Resurrect ticky code for counting constructor arity ----------------------------------------------+---------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: normal | Milestone: Component: Profiling | Version: 7.7 Keywords: | Operating System: Architecture: Unknown/Multiple | Unknown/Multiple Difficulty: Moderate (less than a day) | Type of failure: Other Blocked By: | Test Case: Related Tickets: | Blocking: ----------------------------------------------+---------------------------- There is a dead piece of ticky profiling code that computes histograms of data constructor arities (including tuples) and size of vector returns. The function responsible is `bumpHistogram` in [[GhcFile(compiler/codeGen/StgCmmTicky.hs)]], which masks the commented out function `bumpHistogramE`. We can improve ticky profiling by resurrecting this dead code. There are following things that need to be done: 1. Replace current no-op `bumpHistogram` with `bumpHistogramE`. Note that current implementation of `bumpHistogramE` computes constructor arity at runtime. This is unnecessary because we know arity at compile time so we can avoid runtime check by doing sth like this: {{{ bumpHistogram lbl n = do let offset = n `min` 8 emit (addToMem cLong (cmmIndexExpr cLongWidth (CmmLit (CmmLabel (mkRtsDataLabel lbl))) (CmmReg (CmmInt (fromIntegral offset) cLongWidth))) 1) }}} Note that `mkRtsDataLabel` function does not exist anymore but we should be able to replace that call with `mkCmmDataLabel rtsPackageId lbl` (speculation). 2. We need to declare arrays that will store histogram values. Declarations of variables used for storing ticky statistics are in [[GhcFile(includes/stg/Ticky.h)]]. We also need to initialize the declared array to contain only zeros. 3. Having declared the arrays we need to fix printing of computed arities. This is done in [[GhcFile(rts/Ticky.c)]]. Note that there is a lot of code that is commented out (`/* krc: comment out some of this stuff temporarily */`) or disabled with `#if FALSE` pragma. We need to resurect *some* of it. There is a `PR_HST` macro for printing one histogram entry. This seems bad. We should probably rework the part of code responsible for printing out results of historgams. 4. Current code counts arities from 0 to 8. Everything above 8 is put into the same bin as 8. This magical 8 is spread all over the code. We should declare a constant that is visible both in Haskell sources (see `bumpHistogram` in 1.) and RTS files and have all code behave properly depending on that constant - we should have loops instead of statically printing 9 elements of histogram array. 5. Some ticky functions that count arity histograms are not used. For example `tickyReturnNewCon` should probably be called by `cgConApp` like this: {{{ ; emit =<< fcode_init ; tickyReturnNewCon ; emitReturn [idInfoToAmode idinfo] } }}} The above is a an outline, which should be fairly accurate, but unexpected things may show up along the way. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: new Priority: normal | Milestone: Component: Profiling | Version: 7.7 Resolution: | Keywords: Operating System: | Architecture: Unknown/Multiple Unknown/Multiple | Difficulty: Moderate (less Type of failure: Other | than a day) Test Case: | Blocked By: Blocking: | Related Tickets: Differential Revisions: | -------------------------------------+------------------------------------- Changes (by mlen): * owner: => mlen -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:1> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: new Priority: normal | Milestone: Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: | Architecture: Unknown/Multiple Unknown/Multiple | Difficulty: Moderate (less Type of failure: Other | than a day) Test Case: | Blocked By: Blocking: | Related Tickets: Differential Revisions: | -------------------------------------+------------------------------------- Changes (by jstolarek): * keywords: => newcomer -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:2> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: new Priority: normal | Milestone: Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: | Architecture: Unknown/Multiple Unknown/Multiple | Difficulty: Moderate (less Type of failure: Other | than a day) Test Case: | Blocked By: Blocking: | Related Tickets: Differential Revisions: | -------------------------------------+------------------------------------- Comment (by mlen): The work in progress version is available at: https://github.com/mlen/ghc/tree/ticky Recently I haven't got time to make any progress. I'll probably get something done during the weekend. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:3> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: new Priority: normal | Milestone: Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: | Architecture: Unknown/Multiple Unknown/Multiple | Difficulty: Moderate (less Type of failure: Other | than a day) Test Case: | Blocked By: Blocking: | Related Tickets: Differential Revisions: | -------------------------------------+------------------------------------- Comment (by jstolarek): mlen, any progress on this one? -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:4> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: new Priority: normal | Milestone: Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: | Architecture: Unknown/Multiple Unknown/Multiple | Difficulty: Moderate (less Type of failure: Other | than a day) Test Case: | Blocked By: Blocking: | Related Tickets: Differential Revisions: | -------------------------------------+------------------------------------- Comment (by mlen): No, sorry. I hope I'll be able to take a look at it again soon. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:5> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: new Priority: normal | Milestone: Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Unknown/Multiple | Architecture: Type of failure: Other | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: -------------------------------------+------------------------------------- Comment (by mlen): jstolarek: I made some progress recently: relevant constants (`TICKY_BIN_COUNT`) are defined only in `includes/stg/Ticky.h` and then made available to Haskell code via `utils/deriveConstants`. The last thing that's left is making sure counting functions are used where they are needed and refactoring `rts/Ticky.c` a bit. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:6> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: new Priority: normal | Milestone: Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Unknown/Multiple | Architecture: Type of failure: Other | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: -------------------------------------+------------------------------------- Comment (by jstolarek): Sounds great. Can you create a Phab revision for your work so that I and other developers can review it? -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:7> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: patch Priority: normal | Milestone: Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Unknown/Multiple | Architecture: Type of failure: Other | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: D931 -------------------------------------+------------------------------------- Changes (by mlen): * status: new => patch * differential: => D931 Comment: Sorry for the long delay. Finally I found some time (during ZuriHac) to rework the patch to include tests that the code is really generated and created the differential. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:8> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: patch Priority: normal | Milestone: Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Unknown/Multiple | Architecture: Type of failure: Other | Unknown/Multiple Blocked By: | Test Case: Related Tickets: | Blocking: | Differential Revisions: Phab:D931 -------------------------------------+------------------------------------- Changes (by nomeata): * differential: D931 => Phab:D931 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:9> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: patch Priority: normal | Milestone: Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 -------------------------------------+------------------------------------- Comment (by Ben Gamari <ben@…>): In [changeset:"f0f0ac859257a0b528815adb61d3f024c8bafa16/ghc" f0f0ac8/ghc]: {{{ #!CommitTicketReference repository="ghc" revision="f0f0ac859257a0b528815adb61d3f024c8bafa16" Fix histograms for ticky code This patch fixes Cmm generation required to produce histograms when compiling with -ticky flag, strips dead code from rts/Ticky.c and reworks it to use a shared constant in both C and Haskell code. Fixes #8308. Test Plan: T8308 Reviewers: jstolarek, simonpj, austin Reviewed By: simonpj Subscribers: mpickering, simonpj, bgamari, mlen, thomie, jstolarek Differential Revision: https://phabricator.haskell.org/D931 GHC Trac Issues: #8308 }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:10> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: mlen Type: task | Status: closed Priority: normal | Milestone: 8.2.1 Component: Profiling | Version: 7.7 Resolution: fixed | Keywords: newcomer Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Wiki Page: | -------------------------------------+------------------------------------- Changes (by bgamari): * status: patch => closed * resolution: => fixed * milestone: => 8.2.1 Comment: This is now fixed. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:11> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: normal | Milestone: 8.2.1 Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Wiki Page: | -------------------------------------+------------------------------------- Changes (by simonpj): * status: closed => new * owner: mlen => * resolution: fixed => Comment: It's failing on Linux, when validating. I have {{{ GhcLibHcOpts += -ticky }}} if it makes a difference. {{{ --- /tmp/ghctest/0MzM7D/1/2/3/./rts/T8308/T8308/T8308.stdout.normalised 2016-05-19 11:44:19.368052973 +0100 +++ /tmp/ghctest/0MzM7D/1/2/3/./rts/T8308/T8308/T8308.run.stdout.normalised 2016-05-19 11:44:19.368052973 +0100 @@ -1 +1 @@ -1 +8 *** unexpected failure for T8308(normal) }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:12> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: normal | Milestone: 8.2.1 Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Wiki Page: | -------------------------------------+------------------------------------- Comment (by bgamari): I suspect the issue here is the fact that you built your libraries with ticky. The testcase is expecting to see a count of 1 in the `RET_NEW_hst_1` ticker, since it expects only the testcase itself to be instrumented. However, since you have also instrumented the libraries, the count is larger. In this sense the testcase as-written may be a bit fragile, although I can't think of any great way to fix this without compromising the precision of the check in the "normal" case (where the user has instrumented their libraries, which is by far more common). -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:13> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity -------------------------------------+------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: normal | Milestone: 8.2.1 Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): OK. I can live with that. Can you put a comment in the source file or the all.T file to mention this point? -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:14> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity ------------------------------+---------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: normal | Milestone: 8.2.1 Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Windows | Architecture: Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Wiki Page: | ------------------------------+---------------------------------------- Changes (by thomie): * cc: Phyx- (added) * os: Unknown/Multiple => Windows Comment: On Windows, `T8308` fails with: {{{ Actual stdout output differs from expected: ... -1 +0 *** unexpected failure for T8308(normal) }}} After running `make test TEST=T8308 CLEANUP=0`, the `.ticky` file contains this fishy looking histogram (note the `4294967296` entry): {{{ The following table is explained by http://ghc.haskell.org/trac/ghc/wiki/Debugging/TickyTicky All allocation numbers are in bytes. ************************************************** Entries Alloc Alloc'd Non-void Arguments STG Name -------------------------------------------------------------------------------- 1 16 0 1 I f{v rpA} (main@main:Main) (fun) ************************************************** ... 0 RET_UNBOXED_TUP_ctr 4294967296 RET_NEW_hst_0 0 RET_NEW_hst_1 ... }}} @mlen: any idea? Perhaps a known issue with ticky on Windows? -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:15> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity ------------------------------+---------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: normal | Milestone: 8.2.1 Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Windows | Architecture: Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Wiki Page: | ------------------------------+---------------------------------------- Comment (by mlen): @Phyx-: nope, sorry. I don't have any windows experience. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:16> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity ------------------------------+---------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: new Priority: normal | Milestone: 8.2.1 Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Windows | Architecture: Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Wiki Page: | ------------------------------+---------------------------------------- Comment (by Thomas Miedema <thomasmiedema@…>): In [changeset:"f5f5a8a7957d6c52f47071d2b7419b47e43e9a9d/ghc" f5f5a8a/ghc]: {{{ #!CommitTicketReference repository="ghc" revision="f5f5a8a7957d6c52f47071d2b7419b47e43e9a9d" Testsuite Windows: mark T8308 expect_broken (#8308) }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:17> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity ------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: patch Priority: normal | Milestone: 8.2.1 Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Windows | Architecture: Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Phab:D2318 Wiki Page: | ------------------------------+-------------------------------------------- Changes (by Phyx-): * status: new => patch * differential: Phab:D931 => Phab:D931 Phab:D2318 Comment: I've submitted a patch for Windows and re-enabled the test in Phab:D2318 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:18> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity ------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: patch Priority: normal | Milestone: 8.2.1 Component: Profiling | Version: 7.7 Resolution: | Keywords: newcomer Operating System: Windows | Architecture: Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Phab:D2318 Wiki Page: | ------------------------------+-------------------------------------------- Comment (by Tamar Christina <tamar@…>): In [changeset:"b020db2a841c397a02ec352f8b6dc110b38b927b/ghc" b020db2/ghc]: {{{ #!CommitTicketReference repository="ghc" revision="b020db2a841c397a02ec352f8b6dc110b38b927b" Fix Ticky histogram on Windows Summary: The histogram types are defined in `Ticky.c` as `StgInt` values. ``` EXTERN StgInt RET_NEW_hst[TICKY_BIN_COUNT] INIT({0}); EXTERN StgInt RET_OLD_hst[TICKY_BIN_COUNT] INIT({0}); EXTERN StgInt RET_UNBOXED_TUP_hst[TICKY_BIN_COUNT] INIT({0}); ``` which means they'll be `32-bits` on `x86` and `64-bits` on `x86_64`. However the `bumpHistogram` in `StgCmmTicky` is incrementing them as if they're a `cLong`. A long on Windows `x86_64` is `32-bit`. As such when then value for the `_hst_1` is being set what it's actually doing is setting the value of the high bits of the first entry. This ends up giving us `0b100000000000000000000000000000000` or `4294967296` as is displayed in the ticket on #8308. Since `StgInt` is defined using the `WORD` size. Just use that directly in `bumpHistogram`. Also since `cLong` is no longer used after this commit it will also be dropped. Test Plan: make TEST=T8308 Reviewers: mlen, jstolarek, bgamari, thomie, goldfire, simonmar, austin Reviewed By: bgamari, thomie Subscribers: #ghc_windows_task_force Differential Revision: https://phabricator.haskell.org/D2318 GHC Trac Issues: #8308 }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:19> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity ------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: closed Priority: normal | Milestone: 8.0.2 Component: Profiling | Version: 7.7 Resolution: fixed | Keywords: newcomer Operating System: Windows | Architecture: Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Phab:D2318 Wiki Page: | ------------------------------+-------------------------------------------- Changes (by Phyx-): * status: patch => closed * resolution: => fixed * milestone: 8.2.1 => 8.0.2 Comment: Fixed, should be in `8.0.2` otherwise the values reported by ticky on Windows are incorrect. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:20> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity ------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: merge Priority: normal | Milestone: 8.0.2 Component: Profiling | Version: 7.7 Resolution: fixed | Keywords: newcomer Operating System: Windows | Architecture: Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Phab:D2318 Wiki Page: | ------------------------------+-------------------------------------------- Changes (by thomie): * status: closed => merge -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:21> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#8308: Resurrect ticky code for counting constructor arity ------------------------------+-------------------------------------------- Reporter: jstolarek | Owner: Type: task | Status: closed Priority: normal | Milestone: 8.0.2 Component: Profiling | Version: 7.7 Resolution: fixed | Keywords: newcomer Operating System: Windows | Architecture: Unknown/Multiple Type of failure: Other | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D931 Phab:D2318 Wiki Page: | ------------------------------+-------------------------------------------- Changes (by bgamari): * status: merge => closed Comment: Merged both the ticky fix and the subsequent Windows fix to `ghc-8.0`. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/8308#comment:22> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
participants (1)
-
GHC