janet-lang / janet

A dynamic language and bytecode vm
https://janet-lang.org
MIT License
3.45k stars 223 forks source link

Rework `keys`, `values`, `pairs` #1241

Closed primo-ppcg closed 1 year ago

primo-ppcg commented 1 year ago

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.

sogaiu commented 1 year ago

master (ecc4d80a):

$ 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 (7417e82c):

$ 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 :+1:

sogaiu commented 1 year ago

@primo-ppcg I was working on trying out the changes mentioned in https://github.com/janet-lang/spork/issues/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 commented 1 year ago

Does the test failing happen for you?

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