#8867 closed bug (fixed)

newArray# fails to initialize card table correctly

Reported by: tibbe Owned by: simonmar
Priority: normal Milestone:
Component: Runtime System Version: 7.6.3
Keywords: Cc: simonmar
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

newArray# doesn't initialize the card table correctly. It writes the init value to the card table instead of a zero. That happens to be harmless, but slightly inefficient. Consider the implementation of newArray# in rts/PrimOps.cmm:

stg_newArrayzh ( W_ n /* words */, gcptr init )
{
    W_ words, size;
    gcptr p, arr;

    again: MAYBE_GC(again);

    // the mark area contains one byte for each 2^MUT_ARR_PTRS_CARD_BITS words
    // in the array, making sure we round up, and then rounding up to a whole
    // number of words.
    size = n + mutArrPtrsCardWords(n);
    words = BYTES_TO_WDS(SIZEOF_StgMutArrPtrs) + size;
    ("ptr" arr) = ccall allocate(MyCapability() "ptr",words);
    TICK_ALLOC_PRIM(SIZEOF_StgMutArrPtrs, WDS(n), 0);

    SET_HDR(arr, stg_MUT_ARR_PTRS_DIRTY_info, CCCS);
    StgMutArrPtrs_ptrs(arr) = n;
    StgMutArrPtrs_size(arr) = size;

    // Initialise all elements of the the array with the value in R2
    p = arr + SIZEOF_StgMutArrPtrs;
  for:
    if (p < arr + WDS(words)) {
        W_[p] = init;
        p = p + WDS(1);
        goto for;
    }
    // Initialise the mark bits with 0
  for2:
    if (p < arr + WDS(size)) {
        W_[p] = 0;
        p = p + WDS(1);
        goto for2;
    }

    return (arr);
}

The second for-loop never gets executed, as size < words. The first predicate should check p < arr + SIZEOF_StgMutArrPtrs + WDS(n) and the second should be p < arr + WDS(words).

Change History (4)

comment:1 Changed 14 months ago by tibbe

A possible second bug: we (should) set all mark bits to zero. Why do we then mark the array as dirty by using the stg_MUT_ARR_PTRS_DIRTY_info info pointer?

Last edited 14 months ago by tibbe (previous) (diff)

comment:2 Changed 14 months ago by simonmar

the mark bits and the header are irrelevant for newly allocated arrays, so we could probably omit the initialisation of the card table altogether. I'll look into this.

comment:3 Changed 14 months ago by Johan Tibell <johan.tibell@…>

In 46d05ba03d1491cade4a3fe33f0b8c404ad3c760/ghc:

Fix two issues in stg_newArrayzh

The implementations of newArray# and newArrayArray#, stg_newArrayzh
and stg_newArrayArrayzh, had three issues:

 * The condition for the loop that fills the array with the initial
   element was incorrect. It would write into the card table as
   well. The condition for the loop that filled the card table was
   never executed, as its condition was also wrong. In the end this
   didn't lead to any disasters as the value of the card table doesn't
   matter for newly allocated arrays.

 * The card table was unnecessarily initialized. The card table is
   only used when the array isn't copied, which new arrays always
   are. By not writing the card table at all we save some cycles.

 * The ticky allocation accounting was wrong. The second argument to
   TICK_ALLOC_PRIM is the size of the closure excluding the header
   size, but the header size was incorrectly included.

Fixes #8867.

comment:4 Changed 14 months ago by tibbe

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.