Skip to content

Do not use Obj.Extension_constructor.of_val in Type.Id - #13782

Merged
nojb merged 6 commits into
ocaml:trunkfrom
bclement-ocp:bclement/type_no_extension_constructor_of_val
Feb 6, 2025
Merged

nojb merged 6 commits into
ocaml:trunkfrom
bclement-ocp:bclement/type_no_extension_constructor_of_val

Conversation

@bclement-ocp

Copy link
Copy Markdown
Contributor

The Type.Id.uid function converts a type identifier to an integer by going through Obj.Extension_constructor.of_val. In this context, Obj.Extension_constructor.of_val is just an expensive way to compute the identity function: it performs multiple checks, including multiple calls to caml_obj_tag, in order to determine if it is passed a nullary extension variant, a non-nullary extension variant, or something that is not an extension variant at all (in which case it raises an Invalid_argument exception).

When called with a nullary extension variant (which is just represented as its constructor), Obj.Extension_constructor.of_val is always the identity. Since the usage in Type.Id is always with a nullary extension variant, use Obj.magic as a less expensive identity function.

For context, this came up while optimising code making many lookups in small heterogenous maps, such that it ended up being noticeable in profiling.

The `Type.Id.uid` function converts a type identifier to an integer by
going through `Obj.Extension_constructor.of_val`. In this context,
`Obj.Extension_constructor.of_val` is just an expensive way to compute
the identity function: it performs multiple checks, including multiple
calls to `caml_obj_tag`, in order to determine if it is passed a nullary
extension variant, a non-nullary extension variant, or something that is
not an extension variant at all (in which case it raises an
Invalid_argument exception).

When called with a nullary extension variant (which is just represented
as its constructor), `Obj.Extension_constructor.of_val` is always the
identity. Since the usage in `Type.Id` is always with a nullary
extension variant, use `Obj.magic` as a less expensive identity
function.

For context, this came up while optimising code making *many* lookups in
small heterogenous maps, such that it ended up being noticeable in
profiling.
@lthls

lthls commented Feb 6, 2025

Copy link
Copy Markdown
Contributor

Instead of Obj.magic, can't you just use [%extension_constructor A.Id] ?

@bclement-ocp

Copy link
Copy Markdown
Contributor Author

Probably, I was not aware this existed — it indeed should be much better.

bclement-ocp added a commit to bclement-ocp/oxcaml that referenced this pull request Feb 6, 2025

@nojb nojb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, modulo nit below.

Comment thread Changes Outdated
- #13740: Improve performance of Weak.find_aux
(Josh Berdine, review by Gabriel Scherer)

- #15049: Do not use Obj.Extension_constructor.of_val in Type.Id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit, but Changes entries should be written from the point of view of the reader, not the writer. Here, a random reader who comes across this entry will not know how or why this would be relevant to her. Instead, something like Avoid use of Obj.Extension_constructor.of_val in Type.Id for better performance. or Type.Id: replace Obj.Extension_constructor.of_val by [%extension_constructor] for better performance. (which has the advantage of publicizing [%extension_constructor]) would be better in this respect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated the changes entry to mention [%extension_constructor] and the fact that it improves both performance and type safety compared to Obj.Extension_constructor.of_val.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great, thanks!

@nojb
nojb merged commit 66cae63 into ocaml:trunk Feb 6, 2025
Ekdohibs pushed a commit to bclement-ocp/oxcaml that referenced this pull request Feb 12, 2025
Comment thread Changes
- #13740: Improve performance of Weak.find_aux
(Josh Berdine, review by Gabriel Scherer)

- #15049: Improve performance and type safety of Type.Id by using

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

incorrect number

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, I've pushed a fix to trunk directly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants