I noticed the following four potential problems (unless they are signs of my misunderstanding). I can make a PR for some/all of them if you agree.
-
Shouldn't one also update t.totsize, by t.totsize <- newt.totsize?
-
|
match Weak.get bucket i with |
|
| Some v when v.node = d -> |
|
begin match Weak.get bucket i with |
|
| Some v -> v |
|
| None -> loop (i+1) |
|
end |
|
| _ -> loop (i+1) |
It looks like there is a redundant call to Weak.get (maybe this is due to some limitations in early versions of Weak and its interactions with the GC?). These lines can probably be replaced by:
match Weak.get bucket i with
| Some v when v.node = d -> v
| Some _ | None -> loop (succ i)
(the variant when v.hkey = hkey && v.node = d, which allows fast rejection, is probably unnecessary since the equality v.node = d is efficient: this is precisely the purpose of maximal hashconsing).
|
let next_sz n = min (3*n/2 + 3) (Sys.max_array_length - 1) |
|
let newsz = min (3 * sz / 2 + 3) (Sys.max_array_length - 1) in |
Both values are capped at Sys.max_array_length - 1, but the first concerns array lengths, so can be capped at Sys.max_array_length, while the second concerns bucket sizes, so should be capped at Obj.Ephemeron.max_ephe_length (which is currently Sys.max_array_length - 2). Also, is the growth rate purposefully, or accidentally the same?
- Most integer equality checks in the file use
== (although some use =, with no clear pattern of when which is used). Why not use = everywhere? Or Int.equal?
I noticed the following four potential problems (unless they are signs of my misunderstanding). I can make a PR for some/all of them if you agree.
ocaml-hashcons/hashcons.ml
Line 73 in 9d6a785
Shouldn't one also update
t.totsize, byt.totsize <- newt.totsize?ocaml-hashcons/hashcons.ml
Lines 110 to 116 in 9d6a785
It looks like there is a redundant call to
Weak.get(maybe this is due to some limitations in early versions of Weak and its interactions with the GC?). These lines can probably be replaced by:(the variant
when v.hkey = hkey && v.node = d, which allows fast rejection, is probably unnecessary since the equalityv.node = dis efficient: this is precisely the purpose of maximal hashconsing).ocaml-hashcons/hashcons.ml
Line 64 in 9d6a785
ocaml-hashcons/hashcons.ml
Line 82 in 9d6a785
Both values are capped at
Sys.max_array_length - 1, but the first concerns array lengths, so can be capped atSys.max_array_length, while the second concerns bucket sizes, so should be capped atObj.Ephemeron.max_ephe_length(which is currentlySys.max_array_length - 2). Also, is the growth rate purposefully, or accidentally the same?==(although some use=, with no clear pattern of when which is used). Why not use=everywhere? OrInt.equal?