Opened 6 years ago

Closed 3 years ago

#8308 closed task (fixed)

Resurrect ticky code for counting constructor arity

Reported by: jstolarek Owned by:
Priority: normal Milestone: 8.0.2
Component: Profiling Version: 7.7
Keywords: newcomer Cc: Phyx-
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:

Description

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 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).

  1. We need to declare arrays that will store histogram values. Declarations of variables used for storing ticky statistics are in includes/stg/Ticky.h. We also need to initialize the declared array to contain only zeros.
  1. Having declared the arrays we need to fix printing of computed arities. This is done in 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.
  1. 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.
  1. 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.

Change History (22)

comment:1 Changed 5 years ago by mlen

Owner: set to mlen

comment:2 Changed 5 years ago by jstolarek

Keywords: newcomer added

comment:3 Changed 5 years ago 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.

comment:4 Changed 5 years ago by jstolarek

mlen, any progress on this one?

comment:5 Changed 5 years ago by mlen

No, sorry. I hope I'll be able to take a look at it again soon.

comment:6 Changed 5 years ago 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.

comment:7 Changed 5 years ago by jstolarek

Sounds great. Can you create a Phab revision for your work so that I and other developers can review it?

comment:8 Changed 4 years ago by mlen

Differential Rev(s): D931
Status: newpatch

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.

comment:9 Changed 4 years ago by nomeata

Differential Rev(s): D931Phab:D931

comment:10 Changed 3 years ago by Ben Gamari <ben@…>

In f0f0ac8/ghc:

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

comment:11 Changed 3 years ago by bgamari

Milestone: 8.2.1
Resolution: fixed
Status: patchclosed

This is now fixed.

comment:12 Changed 3 years ago by simonpj

Owner: mlen deleted
Resolution: fixed
Status: closednew

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)

comment:13 Changed 3 years ago 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).

comment:14 Changed 3 years ago 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?

comment:15 Changed 3 years ago by thomie

Cc: Phyx- added
Operating System: Unknown/MultipleWindows

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?

comment:16 Changed 3 years ago by mlen

@Phyx-: nope, sorry. I don't have any windows experience.

comment:17 Changed 3 years ago by Thomas Miedema <thomasmiedema@…>

In f5f5a8a/ghc:

Testsuite Windows: mark T8308 expect_broken (#8308)

comment:18 Changed 3 years ago by Phyx-

Differential Rev(s): Phab:D931Phab:D931 Phab:D2318
Status: newpatch

I've submitted a patch for Windows and re-enabled the test in Phab:D2318

comment:19 Changed 3 years ago by Tamar Christina <tamar@…>

In b020db2/ghc:

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 `0b‭100000000000000000000000000000000‬` 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

comment:20 Changed 3 years ago by Phyx-

Milestone: 8.2.18.0.2
Resolution: fixed
Status: patchclosed

Fixed, should be in 8.0.2 otherwise the values reported by ticky on Windows are incorrect.

comment:21 Changed 3 years ago by thomie

Status: closedmerge

comment:22 Changed 3 years ago by bgamari

Status: mergeclosed

Merged both the ticky fix and the subsequent Windows fix to ghc-8.0.

Note: See TracTickets for help on using tickets.