Running through precheck - job #246

Precheck crashed, but I restarted it.

The test fails (as could be expected) on ARM, ARM64, PowerPC, and System Z.

I think yes, for some applications it is important to be able to accurately measure allocations. It's certainly important for the multicore GC, which needs a similar patch (without this patch, there are holes of uninitialised data in the minor heap, which confuse multicore's promotion).

I don't think the code size increase is significant. I compared the code size before and after on the compiler distribution (full data here). In total, .text sections got 1.7% bigger, with the biggest outlier being asmcomp/x86_dsl.o, whose .text section got 4% bigger.

The test fails (as could be expected) on ARM, ARM64, PowerPC, and System Z.

Thanks! Happy to fix those, but I'll wait until there's consensus on whether this should be merged.

I would add that when we investigated this behaviour at Jane Street a couple of years ago, it was in response to users being confused about why the minor words measurement wasn't accurate -- so this was user-driven rather than just spotting a technicality.

Thanks for the explanations and for the measurements. I buy the "necessary for Multicore OCaml" argument.

The increase in code size is less than I feared. Indeed, for ocamlopt.opt the whole TEXT section increases by less than 1%.

It is possible to reduce the increase to negligible (0.1% overall, 0 for asmcomp/x86_dsl.o) by adding and using a few alternate entry points for caml_call_gc:

In emit_call_gc:

  begin match gc.gc_size with
  | 16 -> emit_call "caml_call_gc_1"
  | 24 -> emit_call "caml_call_gc_2"
  | 32 -> emit_call "caml_call_gc_3"
  | 40 -> emit_call "caml_call_gc_4"
  | 48 -> emit_call "caml_call_gc_5"
  | _  -> I.add (int gc.gc_size) r15;
          emit_call "caml_call_gc"
  end;

In runtime/amd64.S:

FUNCTION(G(caml_call_gc_1))
CFI_STARTPROC
        addq    $16, %r15
        jmp     G(caml_call_gc)
CFI_ENDPROC

(and likewise for the other 4 special entry points).

5 entry points is probably overkill. But as you noticed I'm a stickler for code size.

(I think that @jhjourdan probably looked at this question when he implemented statmemprof, so I'm pinging him in case the changes here imply some required changes for his code -- possibly simplifications.)

@xavierleroy

It is possible to reduce the increase to negligible (0.1% overall, 0 for asmcomp/x86_dsl.o) by adding and using a few alternate entry points for caml_call_gc:

Good idea, thanks!

5 entry points is probably overkill.

I've added entry points for 1/2/3-word allocations, since those are the sizes special-cased when optimising for size with -compact. (If changing 4/5-word allocations makes a significant difference, we should probably change -compact).

@gasche
This is no coincidence! I've started looking at the statmemprof code to try and get it merged (and am in contact with @jhjourdan). I don't think this particular PR should affect statmemprof, though.

I had a very quick look at the other architectures. ARM, ARM64 and System Z can be fixed exactly like x86-64. PowerPC needs more work. I could look into it or we could decide that PowerPC is not worth the effort.

I'd like to make this bug go away entirely, if possible. I'll have a go at fixing the other archs early next week. Is there something especially hairy about allocations on PowerPC?

Is there something especially hairy about allocations on PowerPC?

Just look at asmcomp/powerpc/emit.mlp. The glue code that calls the GC

• comes in 3 different versions (ppc32, ppc64, ppc64le)
• is much bigger than usual (for ppc64 and ppc64le)
• but is shared between all allocation sites, owing to the cool "conditional function call" instruction of PowerPC.

I can see plausible ways to fix the allocation pointer. But there's the extra issue that our ppc32/ppc64 test machine (a PowerMac G5) died recently, leaving only ppc64le as testable.

I think I have access to a system that can do ppc64 and possibly ppc32 as well.

I pushed a fix for PPC directly on this branch. The generated code is quite compact, I'm pleased :-) It was tested on ppc64le, but the changes are not specific to any of the 3 ppc variants, so with luck it should work for ppc32 and ppc64 as well.

I've just pushed an ARM64 patch. I don't have an ARM64 machine handy to test on right now, so I'd like to see whether this passes inria CI.

I'm using another approach: instead of mutating and then fixing young_ptr, this version only mutates young_ptr on successful allocations, doing the intermediate calculations on the result register instead. I think this is simpler, and it causes no code size increase on ARM because the 3-address format means it's just as easy to subtract from young_ptr and store the result elsewhere as to mutate young_ptr.

Incidentally, the arm64 allocation code contains a special sequence to handle allocations of more than 0xfff bytes. The largest possible allocation is 0x808 bytes, so this can never run. Anyone mind if I delete this code path? Or there a possibility of Max_young_wosize increasing significantly?

CI precheck in progress.

Concerning allocations of more than 0xffff bytes: I guess I didn't realize Max_young_wosize is much lower than 0xffff... I'm OK with removing the large allocation case and replace it with an assert.

CI precheck is successful for arm64 and for ppc64le.

@xavierleroy: I don't know if you missed it, or you are secretly working on it, or you checked that there is no problem, but my comment above points at a potential bug in your PowerPC patch when Spacetime is enabled.

For future reference, where should I look up s390x instructions? I got brcl 13 from here.

The full story is the Principles of Operation manual referenced here https://github.com/ocaml/ocaml/blob/trunk/asmcomp/s390x/NOTES.md . But it's a big manual.

I got the "12" by looking at how integer comparisons are compiled already, especially the branch_for_comparison function

The one-line comment suggests what's going on: the branch conditional instruction uses the magic number as a mask against the (eq, lt, gt, ov) flags, branching if any of the flags with a 1 in the mask is set. So, 12 means "branch if eq or lt", and 13 means "branch if eq or lt or overflow". The "overflow" flag is important for FP comparisons, as it stands for "unordered", hence 13 means "branch if not greater than". For integer comparisons, it's not obvious to me whether the "overflow" flag is cleared or left unchanged, so I'd rather not test it.

@gasche
This is no coincidence! I've started looking at the statmemprof code to try and get it merged (and am in contact with @jhjourdan). I don't think this particular PR should affect statmemprof, though.

I think it will, because statmemprof does a precise accounting of the allocation pointer in the minor heap in order to accurately sample memory blocks according to the desired probability distribution. I am certain that the corresponding change will be minimal, though.

MSVC should now be fixed - it's running through precheck

ARM is still failing:

List of failed tests:
    tests/regression/pr7798/'pr7798.ml' with 2 (native) 
    tests/lib-obj/'with_tag.ml' with 1 (native)

I fixed the ARM code generator. 4 too many bytes were allocated in the fast path of Ialloc, turning the minor heap into Swiss cheese.

Thanks for the fix!

Running through precheck again.

Precheck seems happy. However, I pushed two additional commits, as discussed during review,. 90f80dd adds a comment explaining the PowerPC code generation strategy. c6e64b2 simplifies the allocation sequence on ARM64, based on the fact that minor heap allocations are always < 0x10000 bytes in size. So, I'll run precheck again.

(Release pseudo-management on report: this is arguably a bugfix but it is large and touchy, so we are not putting this in 4.09.)