[Performance] Add early checks to loaded_feature_path to make it faster

[itarato]

Benchmark shows that string interpolations in the loop is very expensive. We can improve with .start_with? / .end_with? to shape the string early.

Base version:

def lookup(s1, s2, list)
	list.find do |elem|
		s1 == "#{elem}/#{s2}" || s1 == "#{elem}/#{s2}/foo"
	end
end

Improved version:

def lookup(s1, s2, list)
	prefix = if s1.end_with?("/#{s2}")
		s1.chomp(name)
	elsif s1.end_with?("/#{s2}/foo")
		s1.chomp("/#{s2}/foo")
	else
		return false
	end

	list.include?(prefix) ? prefix : nil
end

Benchmark:

benchmark do
	lookup(
		"/home/john/folder/subfolder/subfolder/rails/active_record/foo",
		"active_record",
		[
			"/home/john/folder/subfolder/subfolder/rails09",
                        # ... lot more
			"/home/john/folder/subfolder/subfolder/rails", # <- correct one
		]
	)
end

# With `jt benchmark script`:
# Base:     1060301.2
# Improved: 5018656.3

And upon looking at the use of this function, the return type is only as useful as a boolean to indicate a find - replacing the string return to a boolean improves another bit from 5018656.3 to 5133049.9 (4.8x compare to current).

Testing this on a rails app single test execution:

jt --use jvm-ce ruby --cpusampler -S bin/rails test test/...

On master's HEAD:

Truffle::FeatureLoader.loaded_feature_path || 1133ms 100.0% || 1133ms 100.0% || ../../../../truffle/truffleruby/src/main/ruby/truffleruby/core/truffle/feature_loader.rb~201-206:6970-7220

On this branch's commit:

Truffle::FeatureLoader.feature_path_loaded? || 186ms 100.0% || 186ms 100.0% || ../../../../truffle/truffleruby/src/main/ruby/truffleruby/core/truffle/feature_loader.rb~201-215:7030-7567

Same with the parent function find_feature_or_file:

Truffle::FeatureLoader.find_feature_or_file || 4697ms   2.2% || 117ms   0.1% || ../../../../truffle/truffleruby/src/main/ruby/truffleruby/core/truffle/feature_loader.rb~79-141:2566-4768

on master and:

Truffle::FeatureLoader.find_feature_or_file || 3628ms   1.8% || 103ms   0.1% || ../../../../truffle/truffleruby/src/main/ruby/truffleruby/core/truffle/feature_loader.rb~79-141:2566-4768

from this PR's commit.

[eregon]

I wonder if we could store the $LOAD_PATH index in the FeatureEntry, and use that as a faster check to avoid the loop (i.e. load_path[index] == path or load_path.include?(path)). OTOH we would need to care about $LOAD_PATH changing (tracked via version), in which case we should recompute the index for the FeatureEntry.

Actually, I think something simpler and easier would be to keep a Hash of the $LOAD_PATH (recomputed by version). And then we can do load_path_hash.include?(path) which seems really nice and simple.

[itarato]

I believe something along that line can be accomplished. I'd rather ship this separately as that cached result change would still need some thinking how to position it properly. Would that work?

[eregon]

Yeah, that's definitely something that can be done separately.

[eregon]

This looks great. I left some comments to improve it further.

I think it would be good to measure it in a real usage scenario too, not just a microbenchmark.
For instance you could measure how long does require 'bundler/setup' takes (e.g. with p Benchmark.realtime { ... }) before and after this change.
Also could you show the difference with --cpusampler=flamegraph? Just sharing the total time for find_feature_or_file, feature_provided?, and loaded_feature_path/feature_path_loaded? before & after would be helpful, no need to share the full flamegraph.

---

BTW there is also Require Profiling.
I thought this could be quite useful here, but unfortunately I think it does not measure Truffle::FeatureLoader.find_feature_or_file, which is what we would want here.
metrics searching only measures Primitive.find_file and metrics require only measures actually loading a file but not the search before.
Maybe we should improve that. OTOH regular CPUSampler should already be able to measure how much Truffle::FeatureLoader.find_feature_or_file takes without anything special.

[eregon]

Could you write a changelog entry for this under performance? Then I think it's good to go.

[itarato]

Could you write a changelog entry for this under performance? Then I think it's good to go.

Certainly. Also adding some more results to the PR description.