Skip to content
New issue

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

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

Already on GitHub? # to your account

Add an allocator and a finalizer used to run the given fn with a pre-allocated resource #25

Merged
merged 4 commits into from
Jan 17, 2022

Conversation

dinosaure
Copy link
Member

This is a draft about #22 which asks to allocate some resources, pass them to the given fn and free them then. They should not mitigate results but I need a double-check from you @craigfe 👍 . Currently, fact.html did not change so much with this PR. I believe that it's fine.

@dinosaure
Copy link
Member Author

Currently, I did not implemented make_indexed_with_resource. Tell me if you need that.

@craigfe
Copy link
Member

craigfe commented Dec 23, 2021

Thanks a lot for this excellent feature. I've ported ocaml-uring to use it here, and this API seems very natural to me.

Two thoughts:

  • I did indeed need make_indexed_with_resource, which I quickly did in this commit. Perhaps the only interesting point is that I needed to pass the int index argument to allocate in order to create a uring of the correct size.

  • The uring resource is very heavy-weight (& individual bench runs are quite fast), and so allocating an array of them for each iteration inside the time quota quickly crashed the benchmark with ENOMEM. I changed the implementation to instead share a single resource over a run in this commit, and this works for the ocaml-uring usecase. I imagine it's still useful to have distinct resources in some cases, so I'm not sure what to recommend here. (Do we want both behaviours? Is there an API to unify them?)

@dinosaure
Copy link
Member Author

The uring resource is very heavy-weight (& individual bench runs are quite fast), and so allocating an array of them for each iteration inside the time quota quickly crashed the benchmark with ENOMEM. I changed the implementation to instead share a single resource over a run in this commit, and this works for the ocaml-uring usecase. I imagine it's still useful to have distinct resources in some cases, so I'm not sure what to recommend here. (Do we want both behaviours? Is there an API to unify them?)

Yeah I think your case is legitim and the initial case (one resource per run) is legitim too. I will try to merge your commit and prepare something on the API level. Thanks for your feedback, it helps a lot.

@dinosaure
Copy link
Member Author

/cc @craigfe I finalize the API which gives you the choice between the allocation of a single resource or multiple resources. I need to check a bit the assembly generated and if OCaml is able to inline correctly <.> (but I think it can not) to be sure that we don't do some jump/extra allocation/etc.

Can you test this new version with your usecase and see if something is wrong about results? Better, can you give me the link to the repository to reproduce on my side?

@dinosaure
Copy link
Member Author

I did a deep look on the assembly generated by the compiler and it seems that if we use multiple resources for each run, we can trigger the GC via the unsafe_array_get operation. That mostly means that it's not so bad and in your context, you should not have noises around the execution of your function - however, we should precise that "a resource per run" can lead some noises at some points.

@dinosaure
Copy link
Member Author

Finally, if you agree with this interface, I can merge it and prepare a release then 👍.

Copy link
Member

@craigfe craigfe left a comment

Choose a reason for hiding this comment

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

Thanks for this improvement! LGTM.

@dinosaure dinosaure merged commit 8e1a3e6 into master Jan 17, 2022
@dinosaure dinosaure deleted the finalize branch January 17, 2022 09:43
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Jan 17, 2022
… (0.2.0)

CHANGES:

- Add missing dependencies (@kit-ty-kate, mirage/bechamel#20)
- Upgrade the codebase (@dinosaure, mirage/bechamel#24)
- Add a finalizer function to allow the user to allocate
  resource (@dinosaure, @craigfe, mirage/bechamel#22, mirage/bechamel#25)
- Update the documentation (@dinosaure, mirage/bechamel#26)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants