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

Cascade <| functions is slow #6269

Closed
2 of 4 tasks
JaroslavTulach opened this issue Apr 13, 2023 · 4 comments · Fixed by #6384
Closed
2 of 4 tasks

Cascade <| functions is slow #6269

JaroslavTulach opened this issue Apr 13, 2023 · 4 comments · Fixed by #6384

Comments

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Apr 13, 2023

Analysis of "if then else cascade slowdown" concluded that not only we want to continue to convert the conditional statement into if_then_else builtin (and run it as fast as #6255 does), but we also want regular, user written functions like <| or other control flow functions (with no local variables) to run as fast as the if_then_else builtin.

Checklist

  • Write a benchmark comparing cascade of <| functions
  • Improve the speed of any functions that has no local variables by implementing InlineableRootNode
  • Verify unit tests continue to work
  • Verify benchmarks are faster

Dependencies

Follow Up

@JaroslavTulach
Copy link
Member Author

Benchmark shows that cascade of <| is indeed slow - 20x slower than the same code without <|.

@enso-bot
Copy link

enso-bot bot commented Apr 22, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-04-21):

Progress: - benchmark: #6269 (comment)

Next Day: Continue with <| functions

Discord
Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.

@JaroslavTulach
Copy link
Member Author

The idea of inlining "regular functions" cannot be implemented right now:

  • the first thing a function does is to convert arguments into VirtualFrame "slots"
  • that means we need own VirtualFrame even for functions with just parameters
  • or we need to modify the logic to read the local arguments directly from Object[] of arguments
  • however that would complicate the logic[1] and
  • it is not going to work for arguments with default values

Luckily it has been found that <| is implemented as ApplicationOperator builtin - e.g. we can speed it up using similar trick as in #6255 - alas the operator takes VirtualFrame as its argument - ironically that means we may need - # #6293

It has also been concluded that we want to move as much of these <<, >>, <| functions to Function and not have it in Any.enso.

[1] We would need to define our own:

interface CallFrame {
  Object[] getArguments();
  VirtualFrame getFrame();
}

and use it everywhere we use VirtualFrame. That's too much changes for current phase of the release.

@JaroslavTulach JaroslavTulach linked a pull request Apr 24, 2023 that will close this issue
3 tasks
@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to 🔧 Implementation in Issues Board Apr 24, 2023
@JaroslavTulach JaroslavTulach moved this from 🔧 Implementation to 👁️ Code review in Issues Board Apr 24, 2023
@JaroslavTulach JaroslavTulach changed the title Cascade of regular functions like <| is slow Cascade <| functions is slow Apr 24, 2023
@enso-bot
Copy link

enso-bot bot commented Apr 25, 2023

Jaroslav Tulach reports a new STANDUP for yesterday (2023-04-24):

Progress: - PR for <| ready for review: #6384

Next Day: Integrate <| function improvements

Discord
Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.

@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants