Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[WIP] Add auto-complete suggestions #270

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

Draft
pete-murphy wants to merge 4 commits into purescript:master
base: master
Choose a base branch
Loading
from pete-murphy:pm/32/add-auto-complete

Conversation

@pete-murphy
Copy link
Contributor

@pete-murphy pete-murphy commented Feb 6, 2022
edited
Loading

Description of the change

Fixes #32.

Clearly and concisely describe the purpose of the pull request. If this PR relates to an existing issue or change proposal, please link to it. Include any other background context that would help reviewers understand the motivation for this PR.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0 by @)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

Comment on lines 185 to 212
get "/complete" $ do
query <- param "q"
Scotty.setHeader "Access-Control-Allow-Origin" "*"
Scotty.setHeader "Content-Type" "application/json"
let ideClient =
Process.createProcess_ "purs-ide-client"
(Process.proc "purs" ["ide", "client"])
{ Process.std_in = Process.CreatePipe
, Process.std_out = Process.CreatePipe
}
mkCommand q = A.encode $ A.object
[ "command" .= ("complete" :: Text)
, "params" .= A.object
[ "filters" .= A.Array
( V.fromList
[ A.object
[ "filter" .= ("prefix" :: Text)
, "params" .= A.object
[ "search" .= q ]
]
]
)
]
]
(Just handleIn, Just handleOut, _, _) <- liftIO ideClient
liftIO $ Char8.hPutStrLn handleIn (mkCommand (query :: Text))
result <- liftIO $ BS.hGetContents handleOut
Scotty.text (TL.fromStrict (T.decodeUtf8 result))
Copy link
Contributor Author

@pete-murphy pete-murphy Feb 6, 2022

Choose a reason for hiding this comment

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

This currently works, but there might be some inefficiency here that I'm not spotting. Running purs ide client directly (talking to same purs ide server instance) is instantaneous, while hitting this endpoint takes around 2s, sometimes longer.

2022年02月06日 16 00 04

Copy link
Contributor Author

@pete-murphy pete-murphy Feb 6, 2022

Choose a reason for hiding this comment

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

Maybe this could be a POST request with JSON constructed on the client, and passed directly to purs ide client

Copy link
Contributor Author

@pete-murphy pete-murphy Feb 8, 2022
edited
Loading

Choose a reason for hiding this comment

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

It's somehow much faster using POST 7496700, but I don't know if there's any security issues with just proxying directly to purs ide client / purs ide server (probably not since I think purs ide client will fail on malformed input?)

Copy link
Member

@nwolverson -- I'm curious if you have any thoughts on this from the language server side of things; I'm not really sure what is and isn't possible with psc-ide to make this work.

Copy link

As per discussion on discord I'd suggest avoiding proxying the purs ide requests directly simply for the reason that it likely allows read/write access to the entire filesystem as the current user, and probably allows more significant DoS potential than a more directed endpoint. However returning an unparsed blob of json to be parsed on the client side seems like a legitimate option if it avoids re-encoding results & can reuse code.

It is surprising that the request would take 2s, it would seem the only extra overhead is an HTTP request, you are spawning the purs binary from the shell as well as the Haskell code. Are you actually looking at the same request in the same workspace? I see groupReexports and maxResults on the left, I'd also prefer to see a better timing (eg just directly using time).
Certainly making the socket request that purs ide client makes directly could be faster but that's something to shave off milliseconds I'd assume.

Happy to discuss the details of the completion results themselves later - but that request looks sane at least

Copy link
Contributor Author

pete-murphy commented Feb 9, 2022
edited
Loading

This is surprising to me—these are the results from this commit 0bdd1fd (which is back to being a GET request, encoding the JSON on the server)

❯ hyperfine --warmup 1 "curl -Gs --data-urlencode 'q=app' localhost:8081/complete"
Benchmark #1: curl -Gs --data-urlencode 'q=app' localhost:8081/complete
 Time (mean ± σ): 173.3 ms ± 10.2 ms [User: 7.3 ms, System: 11.3 ms]
 Range (min ... max): 155.2 ms ... 188.8 ms 16 runs

so, pretty snappy, but removing this line

liftIO (IO.hSetBuffering handleIn IO.NoBuffering)
seems to result in the 2s slowdown I was seeing before
❯ hyperfine --warmup 1 "curl -Gs --data-urlencode 'q=app' localhost:8081/complete"
Benchmark #1: curl -Gs --data-urlencode 'q=app' localhost:8081/complete
 Time (mean ± σ): 2.339 s ± 0.118 s [User: 5.0 ms, System: 8.3 ms]
 Range (min ... max): 2.209 s ... 2.590 s 10 runs

I added that line because the previous iteration with the POST request was hanging and that seemed to fix it, but I don't fully understand the behavior there.

Other results for completeness :)

From 7496700 POST request (not much difference)

Benchmark #1: curl -s 'http://localhost:8081/complete' -X POST --data-binary '{ "command": "complete", "params": { "filters": [{ "filter": "prefix", "params": { "search": "app" } }] } }' -H 'content-type: application/json'
 Time (mean ± σ): 117.7 ms ± 10.5 ms [User: 4.7 ms, System: 7.0 ms]
 Range (min ... max): 100.6 ms ... 137.3 ms 21 runs

Running purs ide client directly

Benchmark #1: echo '{ "command": "complete", "params": { "filters": [{ "filter": "prefix", "params": { "search": "app" } }] } }' | purs ide client -p 8082
 Time (mean ± σ): 69.8 ms ± 3.2 ms [User: 7.3 ms, System: 16.2 ms]
 Range (min ... max): 65.1 ms ... 81.3 ms 33 runs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Autocomplete

AltStyle によって変換されたページ (->オリジナル) /