Make patricia trees big-endian #3438
Conversation
This patch switches up the implementation of the `Patricia_tree` module from little-endian to big-endian, with the main motivation to be able to implement in-order traversal. The `caml_int_clz_tagged_to_untagged` and `caml_int_tagged_to_tagged` C stubs are recognized and replaced with the `clz` instruction when compiling with flambda2, so they are only used in the boot compiler.
There was a problem hiding this comment.
Looks good, just a few thoughts. Also, someone else should look at the C code since I have very little context there.
One additional change that would be good: negative keys now matter in lots more places, and the current generate_key in patricia_tree_tests.ml rarely generates them (basically only the special cases -1 and min_int ever occur). I suggest changing generate_key by giving the log_int branch a 50% chance of negating the key.
| if bit < 0 | ||
| then ( | ||
| unsigned_iter f t1; | ||
| unsigned_iter f t0) | ||
| else ( | ||
| unsigned_iter f t0; | ||
| unsigned_iter f t1) |
There was a problem hiding this comment.
We're doing this enough times that it seems worth writing something like
let [@inline always] order_branches bit t0 t1 =
if bit < 0 then t1, t0 else t0, t1
and using it here (and other places). (I'd love to find a way to generalize this unsigned_x/x pattern and not have to write things twice, but I can't think of a good one.)
| | Leaf (j, d) -> | ||
| let iv = is_value_of t in | ||
| if i = j | ||
| then empty iv, (found [@inlined hint]) d, empty iv |
There was a problem hiding this comment.
This doesn't seem worth being careful about inlining anymore (and we'd need to do a bit of massaging to make inlining found work anyway), so we can be rid of the [@inlined hint] here.
|
@gretay-js could you please review the C stubs? |
|
|
||
| type key = int | ||
|
|
||
| external int_clz : int -> (int[@untagged]) | ||
| = "caml_int_clz_tagged_to_tagged" "caml_int_clz_tagged_to_untagged" | ||
|
|
There was a problem hiding this comment.
This external declaration should have following attributes:
[@@noalloc] [@@builtin] [@@no_effects] [@@no_coeffects]
In particular, without [@@builtin] this is not going to be converted to a single instruction.
The symbols in builtin_stubs.c will be defined multiple times if an executable has both compiler-libs and ocaml_intrinsics_kernel library as dependencies. One way around it is to declare the symbols in builtin_stubs.c as "weak". Another is to rename these symbols if for performance purposes it doesn't have to be an instruction and a C call is enough.
There was a problem hiding this comment.
The performance is critical for these.
This patch switches up the implementation of the
Patricia_treemodule from little-endian to big-endian, with the main motivation to be able to implement in-order traversal. This also enables more efficient implementation formin_binding,max_binding, andsplit.The
caml_int_clz_tagged_to_untaggedandcaml_int_tagged_to_taggedC stubs are recognized and replaced with theclzinstruction when compiling with flambda2, so they are only used in the boot compiler.Note: this patch changes the implementation of various iteration functions to iterate in signed sorted order, by wrapping unsigned iteration functions, and the tests are updated to reflect that
to_listandto_seqnow return their bindings in (signed) sorted order. In-order traversal will only be needed through a new iterator API (to be added in a separate PR) and I think it would be confusing to have different interfaces use different orders; I am open to removing these wrappers if preferred.