Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Field: More efficient conversion into Bignum_bigint.t #14599

Merged
merged 6 commits into from
Dec 4, 2023
Merged

Field: More efficient conversion into Bignum_bigint.t #14599

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3022675
When comparing `Field.t` values, avoid the conversion to bigint
tizoc Nov 22, 2023
5651bce
Use `into_repr()` when comparing fields in Rust to ensure compatibili…
tizoc Nov 22, 2023
6fcd939
Field: More efficient conversion of into Bignum_bigint.t
tizoc Nov 22, 2023
b570c8f
Do not use binprot serialization when converting bigint fields, use Z…
tizoc Nov 27, 2023
baa6986
Merge branch 'rampup' into optimization/efficient-bignum-bigint-conv
deepthiskumar Dec 2, 2023
498eb98
Merge branch 'rampup' into optimization/efficient-bignum-bigint-conv
deepthiskumar Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 26 additions & 14 deletions src/lib/crypto/kimchi_backend/common/field.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ module type Input_intf = sig

val is_square : t -> bool

val compare : t -> t -> int

val equal : t -> t -> bool

val print : t -> unit
Expand Down Expand Up @@ -177,24 +179,34 @@ module Make (F : Input_intf) :
let of_sexpable = of_bigint
end)

let zero = of_int 0

let to_bignum_bigint n =
let rec go i two_to_the_i acc =
if Int.equal i size_in_bits then acc
else
let acc' =
if Bigint.test_bit n i then Bignum_bigint.(acc + two_to_the_i)
else acc
in
go (i + 1) Bignum_bigint.(two_to_the_i + two_to_the_i) acc'
in
go 0 Bignum_bigint.one Bignum_bigint.zero

let hash_fold_t s x =
Bignum_bigint.hash_fold_t s (to_bignum_bigint (to_bigint x))
(* For non-zero values, conversion is done by creating the bin_prot representation
of the [Bignum_bigit.t] value, and then parsing it with bin_prot. *)
match compare n zero with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? The bigint representation will always be positive, so it should be fine (and actually more efficient) to just special-case zero using an equality check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrmr1993 not at all, the only purpose of all this logic is to:

  1. Recover the sign because bin_prot requires it
  2. Avoid bypassing bignum when converting, but if that is not an issue I can use Zarith directly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrmr1993 btw, are there any values that are common and are worth short-circuiting like zero? do you think it is worth adding one too ?

| 0 ->
Bignum_bigint.zero
| c ->
(* Tag for positive values is 1, negative is 2:
https://github.com/janestreet/bignum/blob/6c63419787a4e209e85befd3d823fff2790677e0/bigint/src/bigint.ml#L27-L30 *)
let tag_byte = if c > 0 then '\x01' else '\x02' in
let bytes = to_bytes n in
let len = Bytes.length bytes in
let size_byte = Char.of_int_exn len in
let buf = Bytes.create (2 + len) in
(* First byte is the tag, second the amount of bytes *)
Bytes.unsafe_set buf 0 tag_byte ;
Bytes.unsafe_set buf 1 size_byte ;
(* Copy the bytes representation of the value, skip the tag and size bytes *)
Bytes.unsafe_blit ~src:bytes ~src_pos:0 ~dst_pos:2 ~len ~dst:buf ;
Bin_prot.Reader.of_bytes Bignum_bigint.Stable.V1.bin_reader_t buf
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you not use Z.of_bits directly? There should be a no-op converter to/from Bigint.t from there.


let hash_fold_t s x = Bignum_bigint.hash_fold_t s (to_bignum_bigint x)

let hash = Hash.of_fold hash_fold_t

let compare t1 t2 = Bigint.compare (to_bigint t1) (to_bigint t2)
let compare = compare

let equal = equal

Expand Down
4 changes: 2 additions & 2 deletions src/lib/crypto/kimchi_backend/kimchi_backend.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ module Kimchi_backend_common : sig

val sexp_of_t : t -> Sexplib0.Sexp.t

val compare : t -> t -> int

val bin_size_t : t Bin_prot.Size.sizer

val bin_write_t : t Bin_prot.Write.writer
Expand Down Expand Up @@ -56,6 +54,8 @@ module Kimchi_backend_common : sig

val is_square : t -> bool

val compare : t -> t -> int

val equal : t -> t -> bool

val print : t -> unit
Expand Down
4 changes: 2 additions & 2 deletions src/lib/crypto/kimchi_bindings/stubs/src/arkworks/pasta_fp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ impl CamlFp {
unsafe extern "C" fn ocaml_compare(x: ocaml::Raw, y: ocaml::Raw) -> i32 {
let x = x.as_pointer::<Self>();
let y = y.as_pointer::<Self>();
match x.as_ref().0.cmp(&y.as_ref().0) {
match x.as_ref().0.into_repr().cmp(&y.as_ref().0.into_repr()) {
core::cmp::Ordering::Less => -1,
core::cmp::Ordering::Equal => 0,
core::cmp::Ordering::Greater => 1,
Expand Down Expand Up @@ -240,7 +240,7 @@ pub fn caml_pasta_fp_mut_square(mut x: ocaml::Pointer<CamlFp>) {
#[ocaml_gen::func]
#[ocaml::func]
pub fn caml_pasta_fp_compare(x: ocaml::Pointer<CamlFp>, y: ocaml::Pointer<CamlFp>) -> ocaml::Int {
match x.as_ref().0.cmp(&y.as_ref().0) {
match x.as_ref().0.into_repr().cmp(&y.as_ref().0.into_repr()) {
Less => -1,
Equal => 0,
Greater => 1,
Expand Down
4 changes: 2 additions & 2 deletions src/lib/crypto/kimchi_bindings/stubs/src/arkworks/pasta_fq.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ impl CamlFq {
unsafe extern "C" fn ocaml_compare(x: ocaml::Raw, y: ocaml::Raw) -> i32 {
let x = x.as_pointer::<Self>();
let y = y.as_pointer::<Self>();
match x.as_ref().0.cmp(&y.as_ref().0) {
match x.as_ref().0.into_repr().cmp(&y.as_ref().0.into_repr()) {
core::cmp::Ordering::Less => -1,
core::cmp::Ordering::Equal => 0,
core::cmp::Ordering::Greater => 1,
Expand Down Expand Up @@ -241,7 +241,7 @@ pub fn caml_pasta_fq_mut_square(mut x: ocaml::Pointer<CamlFq>) {
#[ocaml_gen::func]
#[ocaml::func]
pub fn caml_pasta_fq_compare(x: ocaml::Pointer<CamlFq>, y: ocaml::Pointer<CamlFq>) -> ocaml::Int {
match x.as_ref().0.cmp(&y.as_ref().0) {
match x.as_ref().0.into_repr().cmp(&y.as_ref().0.into_repr()) {
Less => -1,
Equal => 0,
Greater => 1,
Expand Down