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

Rework keys, values, pairs #1241

Merged
merged 1 commit into from
Aug 6, 2023
Merged

Conversation

primo-ppcg
Copy link
Contributor

These functions may not be overly important, but that doesn't mean that they can't be optimized. The main performance gain here is creating an array with the correct capacity before iteration begins, rather than pushing to an empty array.

(use spork/test)

(def xs (range 1000))

(timeit-loop [:timeout 1] "keys  " (keys xs))
(timeit-loop [:timeout 1] "values" (values xs))
(timeit-loop [:timeout 1] "pairs " (pairs xs))

master:

keys   1.000s, 36.86µs/body
values 1.000s, 42.86µs/body
pairs  1.000s, 84.27µs/body

branch:

keys   1.000s, 14.75µs/body
values 1.000s, 19.11µs/body
pairs  1.000s, 56.36µs/body

keys and values are each over twice as fast, and pairs is approximately 50% faster.

Copy link
Member

@pepe pepe left a comment

Choose a reason for hiding this comment

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

LGTM

@primo-ppcg primo-ppcg mentioned this pull request Aug 6, 2023
@sogaiu
Copy link
Contributor

sogaiu commented Aug 6, 2023

master (ecc4d80):

$ janet keys-values-pairs.janet 
keys   1.000s, 40.55µs/body
values 1.000s, 46.86µs/body
pairs  1.000s, 104.3µs/body

branch (7417e82):

$ JANET_PATH=$HOME/.local/lib/janet ~/src/janet.primo-ppcg/build/janet keys-values-pairs.janet 
keys   1.000s, 22.14µs/body
values 1.000s, 29.3µs/body
pairs  1.000s, 81.62µs/body

Also tested against some repositories with no issues 👍

@bakpakin bakpakin merged commit 06eea74 into janet-lang:master Aug 6, 2023
8 checks passed
@primo-ppcg primo-ppcg deleted the keys-values-pairs branch August 6, 2023 13:23
@sogaiu
Copy link
Contributor

sogaiu commented Aug 11, 2023

@primo-ppcg I was working on trying out the changes mentioned in janet-lang/spork#147 and noticed that not all of spork's tests were passing [1]:

running test/suite0011.janet ...
error: expected string, symbol, keyword, array, tuple, table, struct or buffer, got <fiber 0x55A7F3F63840>
  in values [boot.janet] on line 1579, column 30
  in _thunk [test/suite0011.janet] (tailcall) on line 14, column 25
non-zero exit code in test/suite0011.janet: 1

I think the test code in question is:

(assert (deep= @[1 2 3] (values s)))

Does the test failing happen for you?

When I investigated a bit, I found that the test failing occurs after this PR was merged.

Would you mind taking a closer look?


[1] /me makes note to run spork's tests in the future.

@primo-ppcg
Copy link
Contributor Author

Does the test failing happen for you?

Indeed, I've already prepared a branch that adds lengthable? as mentioned here.

@sogaiu sogaiu mentioned this pull request Aug 11, 2023
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