Skip to content

feat(collections/unstable): support Iterable argument in sample() #6035

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

Merged
merged 5 commits into from
Sep 24, 2024

Conversation

Liam-Tait
Copy link
Contributor

@Liam-Tait Liam-Tait commented Sep 23, 2024

Create an unstable version of collections sample(). This version supports both arrays and iterables.

Refactored away the randomInteger function in favour of doing it inline. This is because:

  • no need to add and subtract lower (always 0)
  • no need to add and subtract 1 as we want an index anyway
  • imo it is simpler to read

It is hard to compare in the the diff in the pull request
Here is a simplified version to see the general difference

function randomInteger(lower: number, upper: number): number {
  return lower + Math.floor(Math.random() * (upper - lower + 1));
}
array[randomInteger(0, length - 1)]
// vs
const randomIndex = Math.floor(Math.random() * length);
array[randomIndex];

I considered also adding a special case for string input using String.prototype.split("") because that is faster than Array.from(iterable), however I decided against it because philosophically splitting is not using the string iterator even if it the produces the same.
It also required casting typescript types.

} else if (typeof iterable === "string") {
  array = (iterable as string).split("") as T[];
}
The perf difference with the string case (on my machine)

    CPU | Intel(R) Core(TM) i5-5257U CPU @ 2.70GHz
Runtime | Deno 1.46.3 (x86_64-apple-darwin)

benchmark time/iter (avg) iter/s (min … max) p75 p99 p995


(unstable) sample with string case 1.4 µs 722,500 ( 1.3 µs … 2.0 µs) 1.4 µs 2.0 µs 2.0 µs
(unstable) sample 8.3 µs 121,000 ( 8.1 µs … 8.9 µs) 8.4 µs 8.9 µs 8.9 µs

summary
(unstable) sample with string case
5.97x faster than (unstable) sample

#5470

@Liam-Tait Liam-Tait changed the title feat(collections/sample): support Iterable argument in sample() feat(collections/unstable): support Iterable argument in sample() Sep 23, 2024
@Liam-Tait Liam-Tait marked this pull request as ready for review September 23, 2024 13:21
@Liam-Tait Liam-Tait requested a review from kt3k as a code owner September 23, 2024 13:21
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

Refactored away the randomInteger function in favour of doing it inline.

This looks totally fine to me.

LGTM. Thanks for updating the PR!

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

Successfully merging this pull request may close these issues.

2 participants