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.

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

Adding "Queue.iteri" and "Queue.foldi" functions to the standard library. #1544

Closed
wants to merge 8 commits into from
Closed

Adding "Queue.iteri" and "Queue.foldi" functions to the standard library. #1544

wants to merge 8 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 22, 2017

This PR attempts to:

  • add two new functions:
    • Queue.iteri
    • Queue.foldi
  • rewrite existing Queue.iter and Queue.fold functions in terms of the above "indexed variants"
  • modifies the documentation of the existing Queue.iter and Queue.fold in order to make the description more concrete (i.e. more like List.iter and List.fold_left).

@gasche
Copy link
Member

gasche commented Dec 22, 2017

rewrite existing Queue.iter and Queue.fold functions in terms of the above "indexed variants"

I would not do this without first benchmarking to measure the performance impact. For example, it is possible to compute the maximal value of a queue of non-negative integers with Queue.fold max 0 queue, does your new implementation change the performance of that computation (with and without -flambda, maybe?)?

(For many libraries I would agree that removing redundancy of implementation is a good choice, but standard libraries actually get to decide to be a bit less pleasant within if they can speedup the programs of their users in practically relevant ways.)

@ghost
Copy link
Author

ghost commented Dec 22, 2017

I would not do this without first benchmarking to measure the performance impact. For example, it is possible to compute the maximal value of a queue of non-negative integers with Queue.fold max 0 queue?

I've added now tests for both of the added functions. If a more comprehensive testing is needed, I can expand them further.

Does your new implementation change the performance of that computation (with and without -flambda, maybe?)?

I've made some micro-benchmarks for Queue.iter (to compare the original as well as the changed implementation). The benchmark is extreme in the sense that I don't actually do anything with the values of the queue, I just iterate over them, in order to measure Queue.iter as much as possible instead of measuring other things. The results are:

INFO: ocaml_flavor = no_flambda
INFO:  benchmark = iter
INFO:   queue_size = 1000000
INFO:    executable_type = byte
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 50.00%
INFO:    executable_type = opt
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 0.00%
INFO:   queue_size = 10000000
INFO:    executable_type = byte
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 40.43%
INFO:    executable_type = opt
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 12.50%
INFO:   queue_size = 100000000
INFO:    executable_type = byte
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 40.08%
INFO:    executable_type = opt
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 15.66%
INFO: ocaml_flavor = with_flambda
INFO:  benchmark = iter
INFO:   queue_size = 1000000
INFO:    executable_type = byte
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 75.00%
INFO:    executable_type = opt
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 0.00%
INFO:   queue_size = 10000000
INFO:    executable_type = byte
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 40.43%
INFO:    executable_type = opt
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 0.00%
INFO:   queue_size = 100000000
INFO:    executable_type = byte
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 40.08%
INFO:    executable_type = opt
INFO:     proportional difference (i.e. (min_new - min_orig) / min_orig * 100 %) = 1.20%

E.g., if

  • I took a queue with 10^8 elements and iterated over them
  • used Ocaml with -flambda enabled

then the new Queue.iter implementation is 40% slower than the original one (in case of bytecoded implementation) and 1.2% slower in case of the native executable.

(For many libraries I would agree that removing redundancy of implementation is a good choice, but standard libraries actually get to decide to be a bit less pleasant within if they can speedup the programs of their users in practically relevant ways.)

It is up to you to decide. I have split the original commits so that if you say that if it is absolutely unacceptable to degrade the performance of the original Queue.iter and Queue.fold functions (in order to reduce boiler-plate code), then those two commits can be removed from this PR.

@ghost ghost closed this Jan 23, 2018
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Co-authored-by: Christine Rose <christinerose@users.noreply.github.com>
Co-authored-by: Thibaut Mattio <thibaut.mattio@gmail.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants