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

spread last call argument v2 #302

Merged
merged 4 commits into from
Jun 8, 2020
Merged

spread last call argument v2 #302

merged 4 commits into from
Jun 8, 2020

Conversation

ozanh
Copy link
Contributor

@ozanh ozanh commented Jun 7, 2020

This PR is created in favor of feedback in #299

  • No new OpCode is introduced.
  • OpCall has two operands now. Existing compiled scripts do not work, recompilation is required.
  • OpCall second operand is used to denote whether last stack object is expanded/spread or not, which must be valid tengo Object *Array, *ImmutableArray or for custom user types Spreader.

@ozanh ozanh mentioned this pull request Jun 7, 2020
@d5
Copy link
Owner

d5 commented Jun 7, 2020

It looks good to me. But I'm not sure if we need a Spreader interface given that it will always be array type, which is not difficult to compose. Do you have any good use cases for that?

@d5 d5 requested a review from geseq June 7, 2020 20:32
@ozanh
Copy link
Contributor Author

ozanh commented Jun 7, 2020

I don't have a solid use case for Spreader, it can be removed. User can expose any method to return an array.

@d5
Copy link
Owner

d5 commented Jun 7, 2020

Also, I'd appreciate it if you could update documentation as well.

@ozanh
Copy link
Contributor Author

ozanh commented Jun 8, 2020

I will remove Spreader interface and add documentation tomorrow 👍

Ozan HACIBEKIROGLU added 2 commits June 8, 2020 11:59
@ozanh
Copy link
Contributor Author

ozanh commented Jun 8, 2020

I hope docs are OK because I am not good at English. I added some missing notes about CLI and keyword selectors as well. Closes #301, Closes #265

Copy link
Owner

@d5 d5 left a comment

Choose a reason for hiding this comment

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

It looks good to me. @geseq Want to take a look?

docs/tutorial.md Outdated Show resolved Hide resolved
docs/tutorial.md Outdated Show resolved Hide resolved
@d5
Copy link
Owner

d5 commented Jun 8, 2020

@geseq please review this PR and feel free to merge/release. This should be v2.6.0.

@geseq
Copy link
Collaborator

geseq commented Jun 8, 2020

@ozanh can you post the before and after fib(35) benchmarks in cmd/bench/main.go?

@ozanh
Copy link
Contributor Author

ozanh commented Jun 8, 2020

@geseq cmd/bench/main.go results
OLD

-------------------------------------
fibonacci(35)
-------------------------------------
Result:  9227465
Go:      72.629763ms
Parser:  14.569µs
Compile: 75.709µs
VM:      3.015807538s
-------------------------------------
fibonacci(35) (tail-call #1)
-------------------------------------
Result:  9227465
Go:      65.526133ms
Parser:  16.425µs
Compile: 66.364µs
VM:      3.17805909s
-------------------------------------
fibonacci(35) (tail-call #2)
-------------------------------------
Result:  9227465
Go:      230ns
Parser:  16.279µs
Compile: 60.879µs
VM:      25.062µs

NEW

-------------------------------------
fibonacci(35)
-------------------------------------
Result:  9227465
Go:      62.885487ms
Parser:  15.205µs
Compile: 79.682µs
VM:      2.998246515s
-------------------------------------
fibonacci(35) (tail-call #1)
-------------------------------------
Result:  9227465
Go:      64.565828ms
Parser:  16.822µs
Compile: 68.22µs
VM:      3.084532036s
-------------------------------------
fibonacci(35) (tail-call #2)
-------------------------------------
Result:  9227465
Go:      245ns
Parser:  16.211µs
Compile: 57.697µs
VM:      11.635µs

These are my benchstat comparisons using similar fibonacci functions

name      old time/op    new time/op    delta
Fib-8        2.98s ± 1%     2.98s ± 1%    ~     (p=0.436 n=10+10)
FibTC1-8     3.17s ± 1%     3.11s ± 1%  -1.72%  (p=0.000 n=10+10)
FibTC2-8    53.9µs ± 1%    54.0µs ± 1%    ~     (p=0.143 n=10+10)

name      old alloc/op   new alloc/op   delta
Fib-8        313MB ± 0%     313MB ± 0%    ~     (p=0.881 n=9+9)
FibTC1-8     358MB ± 0%     358MB ± 0%  +0.00%  (p=0.000 n=9+10)
FibTC2-8     126kB ± 0%     126kB ± 0%  +0.06%  (p=0.000 n=10+9)

name      old allocs/op  new allocs/op  delta
Fib-8        39.1M ± 0%     39.1M ± 0%  +0.00%  (p=0.003 n=10+9)
FibTC1-8     44.8M ± 0%     44.8M ± 0%  +0.00%  (p=0.000 n=9+10)
FibTC2-8       462 ± 0%       467 ± 0%  +1.08%  (p=0.000 n=10+10)

@geseq geseq merged commit 7834251 into d5:master Jun 8, 2020
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.

None yet

3 participants