Add require diff algo using Trie

[jonnyboyC]

The PR is in relation to issue #261. Initial profiling looks promising as seen here:

master	this pr

The rbspy reports its flamegraph in svg which github doesn't support unfortunately. Both of these profiles were ~8 minutes of solargraph scan on our main repo. The main take away from the current master branch is the wide flat region of the chart corresponding the determine the correct requires is essentially gone in this PR. While this is only one repo, a deep dive into the data shows this snippet went from 31599 profile samples to 174 a ~170X speedup.

Currently this PR doesn't pass a test in spec/language_server/message/text_document/definition_spec.rb. It appears that it doesn't correctly resolve a require. I've been trying to track down the reason but have not been successful so far. The current suite of api_map_spec test pass so there seems to be an edge case that I haven't covered yet. Any help would be appreciated.

[castwide]

One small issue: in the PR, api_map.rb has an errant end keyword at line 220.

A slightly larger concern: the fast_trie gem uses a native extension that doesn't work on JRuby. Support for JRuby has been spotty, but I'd like to maintain it if possible.

I'm not sure what's causing the definition_spec failure, but I'll look into it.

[jonnyboyC]

I'll take a look to see if I can come up with something that doesn't require fast_trie I'm sure there is still a way to speed snippet. I'm still relatively new to ruby, would it be possible to use fast_trie and fallback to the initial implementation if we're not on MRI?

[castwide]

Maybe. We could definitely handle the runtime issues by rescuing LoadError on require 'fast_trie' and using a different method when Trie isn't defined.

The tricky part is how to manage the dependency. JRuby will fail to install the Solargraph gem if any of the runtime dependencies fail. Two possible workarounds:

1. Require the user to install fast_trie manually.
2. Write a gem extension (e.g., in extconf.rb) that tries to install fast_trie and fails gracefully if the platform doesn't support it.

Both options have drawbacks. I'm open to other suggestions.

[jonnyboyC]

I poked around for a bit found a few other native trie extensions, and at least one pure ruby trie. I may try to quickly prototype that today to see how much of a performance difference it is. My guess is it'll likely be slower than the current master implementation.

I think either of the two options you specified make sense to me. Would you think move the method it's own class and pick the strategy based on the presence trie_fast using the rescue on LoadError?

[jonnyboyC]

I've pushed a new commit that takes the approach of your first suggestion going the manual route. I've added a new ReqDiff class that checks if trie-fast is available. If trie-fast is present it uses the new algorithm otherwise we fall back to the hash based approach. I've moved trie-fast out of the the .gemspec and placed it in the Gemfile under the mri platform. I believe that this will install trie-fast when using bundler with mri only. I'll try to install jruby and confirm this is the case.

[castwide]

Travis CI confirms that the installation works on jruby-head. A bunch of other spec errors happen, but they appear to be unrelated (or only semi-related) to the optional fast-trie dependency. There's more work to be done, but I think we're on the right track.

[jonnyboyC]

Good to hear it appears to not blow up on jruby. I'll try get some more time to figure out why some of theses tests are failing soon.

[castwide]

I recently pushed some changes to master that impact this PR. In short, they reduce the amount of duplicated effort ApiMap#catalog performs while resolving requires. This doesn't preclude the potential performance benefits of using a trie, but should at least complement them significantly.

[jonnyboyC]

I've pushed a small push to resolve the conflict with master. I'm still trying to find why this test is failing. I can't say with much confidence but the test may currently be passing in master because of this bug I had mentioned earlier.

In this delete_if block it appears we're checking to see if a require is already in the bundle.workspace.require_paths. For the definition_spec.rb it is loading in the /fixtures/workspace as the test workspace with the following directory structure.

├── lib/
│   ├── other.rb
│   └── thing.rb
├── .solargraph.yml
└── app.rb

In the first test it looks like we're sending the definition request to the server in lib/other.rb for Thing.new.do_thing. Back in api_map.rb it appears the current implementations does not find lib/thing.rb. In the block I linked above we have the following locals

reqs = <Set: {"thing"}>
workspace_path = <Pathname:spec/fixtures/workspace>
base = "spec/fixtures/workspace/lib"
r = "thing"
pn = "spec/fixtures/workspace/spec/fixtures/workspace/lib/thing.rb"

file_keys = [
 "spec/fixtures/workspace/lib/other.rb",
 "spec/fixtures/workspace/lib/thing.rb",
 "spec/fixtures/workspace/app.rb"
]

In this iteration I think we would have intended to remove "thing" from the reqs set and updated local_path_hash

local_path_hash = {"thing"=>"spec/fixtures/workspace/lib/thing.rb"}` 

I believe the test passes because it depends on this block not removing "thing" from reqs. Would there any additional setup calls in the definition_spec.rb that may need to occur?

[castwide]

Suggested change

	local_path_hash = @require_diff.unresolved_requires(bundle, reqs.clone, new_map_hash)
	local_path_hash.merge! @require_diff.unresolved_requires(bundle, reqs.clone, new_map_hash)

[jonnyboyC]

Good catch this should be fixed now as well as a few other potentially problems.

[castwide]

One issue I see is this local_path_hash assignment.

# This creates a local variable instead of replacing `local_path_hash` because
# ApiMap doesn't have a `local_path_hash=` attribute writer.
local_path_hash = @require_diff.unresolved_requires(bundle, reqs.clone, new_map_hash)

# This merges the hash from `@require_diff.unresolved_requires` into `local_path_hash`.
local_path_hash.merge! @require_diff.unresolved_requires(bundle, reqs.clone, new_map_hash)

[jonnyboyC]

@castwide finally had some time to take a look into the into why these test were failing (actually using your vscode debugging extension). I did notice that jruby failed but saw it was in the allowed to fail bucket. I didn't see anything in there related to this Trie change so I'm assuming it was already failing for other reasons.

[mattn]

Any update on this?

[jonnyboyC]

I haven't done anything on this branch since my last update. There was a failing test but it didn't appear to me to have anything to do with my changes so I'd assume it was fine. Likely I need to rebase but to my knowledge this could get merged if when @castwide wants.

[castwide]

Sorry for the delay in getting back to this. I've tested this locally and it seems to work fine, but I don't see any significant change in performance. Can you give me a reproducible example that demonstrates the improvement?

[jonnyboyC]

No worries, let me test it again locally and get some timing. Unfortunately the example I was testing on locally was our private repo which has ~10k files. I may try running this against the rails repo to see if this produces a noticeable speedup