janestreet / janestreet/base_bigstring

`get_uint32_be` / `get_uint32_le` allocate a boxed `Int32` per call

Open Beginner friendly
#8 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

forwarded-to-js-devs
Dominant language
OCaml
Stars
12
Forks
10
PR merge metrics
No merged PRs in 30d

Description

## Summary

`get_uint32_be` and `get_uint32_le` convert through a boxed `Int32.t` and, unlike their
`unsafe_` twins, carry no `[@inline]`. On compilers that cannot eliminate the box across
that call the intermediate survives, costing **3 words per call** — exactly one boxed
`Int32`.

The affected pair differs from the working pair *only* by that annotation.

## The code

`src/base_bigstring.ml`, v0.17.0 (lines 846–867; byte-identical at 1012–1033 in
`v0.18~preview.130.106+341`):

```ocaml
let[@inline] unsafe_get_uint32_le t ~pos =
uint32_of_int32_t (unsafe_get_int32_t_le t ~pos)
;;

let[@inline] unsafe_get_uint32_be t ~pos =
uint32_of_int32_t (unsafe_get_int32_t_be t ~pos)
;;

(* ... and 16 lines later, with no [@inline]: *)

let get_uint32_le t ~pos = uint32_of_int32_t (get_int32_t_le t ~pos)
let get_uint32_be t ~pos = uint32_of_int32_t (get_int32_t_be t ~pos)
```

`uint32_of_int32_t` (line 835) is already `[@inline always]`. `get_int32_t_be` (line 609)
is a closure-valued
binding (`if arch_big_endian then read_int32 else read_int32_swap`), so its boxed `Int32.t`
result cannot be unboxed at a call site that has not itself been inlined.

## Reproduction

Complete and self-contained. `dune-project` with `(lang dune 3.16)`, a `dune` file with
`(executable (name repro) (libraries core base_bigstring))`, and:

```ocaml
open! Core

let n = 1_000_000
let buf = Bigstring.init 64 ~f:(fun i -> Char.of_int_exn ((i * 7) land 0xFF))
let sink = ref 0

let measure name f =
for _ = 1 to 1_000 do sink := !sink lxor f () done; (* warm up *)
let before = Gc.minor_words () in
for _ = 1 to n do sink := !sink lxor f () done;
let after = Gc.minor_words () in
printf "%-26s %5.2f words/call\n" name (Float.of_int (after - before) /. Float.of_int n)
;;

let () =
printf "OCaml %s\n\n" Sys.ocaml_version;
measure "get_uint32_be" (fun () -> Bigstring.get_uint32_be buf ~pos:4);
measure "unsafe_get_uint32_be" (fun () -> Bigstring.unsafe_get_uint32_be buf ~pos:4);
measure "get_uint32_le" (fun () -> Bigstring.get_uint32_le buf ~pos:4);
measure "unsafe_get_uint32_le" (fun () -> Bigstring.unsafe_get_uint32_le buf ~pos:4);
measure "get_int64_be_exn" (fun () -> Bigstring.get_int64_be_exn buf ~pos:0);
measure "get_uint64_be_exn" (fun () -> Bigstring.get_uint64_be_exn buf ~pos:0);
measure "get_int32_be" (fun () -> Bigstring.get_int32_be buf ~pos:4)
;;
```

`Core.Gc.minor_words` returns an `int`, so these counts are exact rather than sampled.

## Measurements

| build | `get_uint32_be` | `unsafe_get_uint32_be` |
|---|---|---|
| `ocaml-base-compiler.5.2.1`, no flambda | **3.00 words/call** | 0.00 |
| `5.2.1+flambda`, dune dev profile | **3.00 words/call** | 0.00 |
| `5.2.1+flambda`, `dune --profile release` | **3.00 words/call** | 0.00 |
| `5.2.1+flambda`, explicit `-O3` | 0.00 | 0.00 |
| `5.2.0+ox` (OxCaml / flambda2), release | 0.00 | 0.00 |

`get_uint32_le` behaves identically to `get_uint32_be` in every row.

## Scope, stated plainly

**This is not universal, and I do not want to overstate it.** flambda at `-O3` eliminates
the box, and so does flambda2 — on OxCaml both variants are free. What is left is still
worth fixing:

- `ocaml-base-compiler` has no flambda at all, and it is what a default `opam switch
create` gives you.
- Dune's `release` profile does **not** pass `-O3`, so even a flambda user doing an
ordinary release build pays it. That row surprised me; I had assumed this was a
non-flambda-only issue until I measured it.

**Also not affected:** `get_int64_be_exn`, `get_int64_le_exn`, `get_uint64_be_exn` and
`get_uint64_le_exn` measure 0.00 words/call everywhere, despite the same apparent shape and
also lacking `[@inline]`. Whatever unboxes those does not apply to the `Int32` path. I have
not chased down why, and I am not suggesting those need changing.

## Where it showed up

A Nasdaq TotalView-ITCH 5.0 parser. A 32-bit field is read for a share count, a price, or
half a 48-bit timestamp in nearly every message, so this was roughly six words per message
in an otherwise allocation-free decode loop — the largest single allocation source in it.

## Suggested fix

```diff
-let get_uint32_le t ~pos = uint32_of_int32_t (get_int32_t_le t ~pos)
-let get_uint32_be t ~pos = uint32_of_int32_t (get_int32_t_be t ~pos)
+let[@inline] get_uint32_le t ~pos = uint32_of_int32_t (get_int32_t_le t ~pos)
+let[@inline] get_uint32_be t ~pos = uint32_of_int32_t (get_int32_t_be t ~pos)
```

This gives the safe accessors the annotation their unsafe twins already carry. Happy to
open it as a PR with DCO sign-off if that is the preferred route.

## Versions

`base_bigstring` v0.17.0 and v0.18~preview.130.106+341 (identical code). OCaml 5.2.1 and
5.2.0+ox, x86-64 Linux (WSL 2).

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/base_bigstring.ml at the get_uint32_le and get_uint32_be bindings around lines 846–867, and compare them with their unsafe counterparts. Add the missing inline annotations, then run the provided allocation reproduction with the relevant OCaml and Dune profiles. Done means both safe accessors no longer allocate the boxed Int32 on the reported builds.

Written by the indexing model from the issue text.

Assessment

Tech stack
ocaml
Domain
performance
Issue type
Bug
Difficulty
1/5
Estimated time
Under an hour
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.