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

feat: aliae performance improvement for Windows cmd clink integration #151

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

drwicid
Copy link

@drwicid drwicid commented Aug 19, 2024

Prerequisites

  • I have read and understood the [contributing guide][CONTRIBUTING.md].
  • The commit message follows the [conventional commits][cc] guidelines.
  • Tests for the changes have been added (for bug fixes / features).
  • Docs have been added/updated (for bug fixes / features).

Performance Issue Defined

The Lua script generated by Aliae uses multiple io.popen calls executed in serial. Each io.popen call takes 20ms when Clink is injected and 50ms when Clink is configured for autorun.

local p = assert(io.popen("doskey tf=terraform $*"))
p:close()
local p = assert(io.popen("doskey tfa=terraform apply $*"))
p:close()
local p = assert(io.popen("doskey tfd=terraform destroy $*"))
p:close()
local p = assert(io.popen("doskey tfv=terraform validate $*"))
p:close()
local p = assert(io.popen("doskey tfi=terraform init $*"))
p:close()

As such, the more aliases you configure, the longer the delay when launching CMD with Clink. With 70 aliases, the Lua script takes 4-7 seconds to execute before the prompt is ready for input.

Proposed Solution

I propose the use of a Doskey macrofile, which will require only one io.popen call, greatly improving the performance with Clink on Windows to a near-instantaneous start time for cmd.exe.

The resulting Aliae syntax generated for clink looks like:

local filename  = os.tmpname()
local macrofile = io.open(filename, "w+")
macrofile:write("tf=terraform $*", "\n")
macrofile:write("tfa=terraform apply $*", "\n")
macrofile:write("tfd=terraform destroy $*", "\n")
macrofile:write("tfv=terraform validate $*", "\n")
macrofile:write("tfi=terraform init $*", "\n")
macrofile:close()
local _ = io.popen(string.format("doskey /macrofile=%s", filename)):close()
os.remove(filename)

Side Effect / Breaking Change

Aliases making use of an ampersand (&) need to be specified differently when using a macrofile versus directly on the command line. As such, the yaml configuration for aliases using & would require updating.

Alias definitions in current state:

alias:
  - name: git-lazy
    value: >- 
      git commit -m "." ^&^& git push
    if: match .Shell "cmd"

Would need to be updated after this code change:

alias:
  - name: git-lazy
    value: >- 
      git commit -m "." && git push
    if: match .Shell "cmd" "zsh" "bash"

Another benefit of the change to a macrofile allows for greater support of cross-platform aliases -- case in point the git alias above requires different syntax for windows in current state, but in the proposed future state, one alias serves many platforms.

A workaround to support the transition could involve simple string replacement of ^& with & to make the change non-breaking. I'm not a fan of hidden string manipulation, so I did not implement it in this PR; however, it could be an effective migration strategy to consider.

Final Thoughts

I believe the performance improvement is worth the risk of a small, but easy to fix, breaking change.

@JanDeDobbeleer
Copy link
Owner

@drwicid thanks for the very useful addition! I'll review once I'm back from my holidays, which is somewhere next week.

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.

2 participants