Add LSI benchmark system for GSL vs native Ruby comparison - #74
Conversation
- Add SUPPRESS_GSL_WARNING=true env var to silence the notice - Update warning message with clear install instructions - Modernize README with platform-specific GSL installation steps - Rename README.markdown to README.md Closes #41
- Add benchmark/lsi_benchmark.rb with performance tests - Add rake benchmark and rake benchmark:compare tasks - Test add_items, build_index, classify, search, find_related - Handle native Ruby SVD numerical instability gracefully - Update Rakefile to reference README.md (renamed from .markdown) Note: Native Ruby SVD has numerical limits with larger document sets. GSL is recommended for production use with large collections.
47ff4a4 to
c1b1b89
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive benchmarking system to compare LSI performance between GSL and native Ruby implementations. The benchmark script measures key operations (adding items, building index, classification, search, and finding related documents) across different document set sizes, with graceful handling of GSL availability and native Ruby SVD numerical stability limitations.
Key Changes:
- New benchmarking infrastructure with single-run and comparison modes
- Enhanced GSL availability warnings with suppression option
- Updated documentation for GSL installation and usage
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| benchmark/lsi_benchmark.rb | Comprehensive benchmark script with subprocess-based comparison system for GSL vs native Ruby performance testing |
| Rakefile | Added benchmark and benchmark:compare rake tasks, updated README reference |
| lib/classifier/lsi.rb | Enhanced GSL warning message with suppression option |
| README.md | Expanded GSL installation instructions and documented warning suppression |
| .rubocop.yml | Excluded benchmark directory from RuboCop checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| puts "\n%-20s %10s %10s %10s" % ['Operation', 'User', 'System', 'Total'] | ||
| puts '-' * 52 | ||
| results.each do |name, bm| | ||
| puts "%-20s %10.4f %10.4f %10.4f" % [name, bm.utime, bm.stime, bm.total] | ||
| end | ||
|
|
||
| total = results.values.sum(&:total) | ||
| puts '-' * 52 | ||
| puts "%-20s %10s %10s %10.4f" % ['TOTAL', '', '', total] |
There was a problem hiding this comment.
Using % for string formatting with array arguments can be hard to read and maintain. Consider using Ruby's format method instead: format(\"\\n%-20s %10s %10s %10s\", 'Operation', 'User', 'System', 'Total')
| puts "\n%-20s %10s %10s %10s" % ['Operation', 'User', 'System', 'Total'] | |
| puts '-' * 52 | |
| results.each do |name, bm| | |
| puts "%-20s %10.4f %10.4f %10.4f" % [name, bm.utime, bm.stime, bm.total] | |
| end | |
| total = results.values.sum(&:total) | |
| puts '-' * 52 | |
| puts "%-20s %10s %10s %10.4f" % ['TOTAL', '', '', total] | |
| puts format("\n%-20s %10s %10s %10s", 'Operation', 'User', 'System', 'Total') | |
| puts '-' * 52 | |
| results.each do |name, bm| | |
| puts format("%-20s %10.4f %10.4f %10.4f", name, bm.utime, bm.stime, bm.total) | |
| end | |
| total = results.values.sum(&:total) | |
| puts '-' * 52 | |
| puts format("%-20s %10s %10s %10.4f", 'TOTAL', '', '', total) |
| puts "\n%-20s %10s %10s %10s" % ['Operation', 'User', 'System', 'Total'] | ||
| puts '-' * 52 | ||
| results.each do |name, bm| | ||
| puts "%-20s %10.4f %10.4f %10.4f" % [name, bm.utime, bm.stime, bm.total] | ||
| end | ||
|
|
||
| total = results.values.sum(&:total) | ||
| puts '-' * 52 | ||
| puts "%-20s %10s %10s %10.4f" % ['TOTAL', '', '', total] |
There was a problem hiding this comment.
Using % for string formatting with array arguments can be hard to read and maintain. Consider using Ruby's format method instead: format(\"%-20s %10.4f %10.4f %10.4f\", name, bm.utime, bm.stime, bm.total)
| puts "\n%-20s %10s %10s %10s" % ['Operation', 'User', 'System', 'Total'] | |
| puts '-' * 52 | |
| results.each do |name, bm| | |
| puts "%-20s %10.4f %10.4f %10.4f" % [name, bm.utime, bm.stime, bm.total] | |
| end | |
| total = results.values.sum(&:total) | |
| puts '-' * 52 | |
| puts "%-20s %10s %10s %10.4f" % ['TOTAL', '', '', total] | |
| puts format("\n%-20s %10s %10s %10s", 'Operation', 'User', 'System', 'Total') | |
| puts '-' * 52 | |
| results.each do |name, bm| | |
| puts format("%-20s %10.4f %10.4f %10.4f", name, bm.utime, bm.stime, bm.total) | |
| end | |
| total = results.values.sum(&:total) | |
| puts '-' * 52 | |
| puts format("%-20s %10s %10s %10.4f", 'TOTAL', '', '', total) |
|
|
||
| total = results.values.sum(&:total) | ||
| puts '-' * 52 | ||
| puts "%-20s %10s %10s %10.4f" % ['TOTAL', '', '', total] |
There was a problem hiding this comment.
Using % for string formatting with array arguments can be hard to read and maintain. Consider using Ruby's format method instead: format(\"%-20s %10s %10s %10.4f\", 'TOTAL', '', '', total)
| puts "%-20s %10s %10s %10.4f" % ['TOTAL', '', '', total] | |
| puts format("%-20s %10s %10s %10.4f", 'TOTAL', '', '', total) |
| next unless native || gsl | ||
|
|
||
| puts "\n--- #{size} Documents ---" | ||
| puts "%-20s %12s %12s %12s" % ['Operation', 'Native', 'GSL', 'Speedup'] |
There was a problem hiding this comment.
Using % for string formatting with array arguments can be hard to read and maintain. Consider using Ruby's format method instead: format(\"%-20s %12s %12s %12s\", 'Operation', 'Native', 'GSL', 'Speedup')
| 'N/A' | ||
| end | ||
|
|
||
| puts "%-20s %12s %12s %12s" % [op, native_str, gsl_str, speedup] |
There was a problem hiding this comment.
Using % for string formatting with array arguments can be hard to read and maintain. Consider using Ruby's format method instead: format(\"%-20s %12s %12s %12s\", op, native_str, gsl_str, speedup)
| gsl_total = operations.sum { |op| gsl[op] } | ||
| speedup = gsl_total > 0 ? native_total / gsl_total : 0 | ||
| puts '-' * 58 | ||
| puts "%-20s %12.4f %12.4f %11.1fx" % ['TOTAL', native_total, gsl_total, speedup] |
There was a problem hiding this comment.
Using % for string formatting with array arguments can be hard to read and maintain. Consider using Ruby's format method instead: format(\"%-20s %12.4f %12.4f %11.1fx\", 'TOTAL', native_total, gsl_total, speedup)
| puts "\n>>> Running Native Ruby benchmarks..." | ||
| sizes.each do |size| | ||
| docs = doc_sets[size] | ||
| docs_literal = docs.map { |d, c| "[#{d.inspect}, #{c.inspect}]" }.join(', ') |
There was a problem hiding this comment.
Constructing code strings by interpolating user-controlled data can lead to code injection vulnerabilities. Consider using a safer approach like passing data through stdin or a temporary file, or using JSON serialization instead of building Ruby literals.
| puts "\n>>> Running GSL benchmarks..." | ||
| sizes.each do |size| | ||
| docs = doc_sets[size] | ||
| docs_literal = docs.map { |d, c| "[#{d.inspect}, #{c.inspect}]" }.join(', ') |
There was a problem hiding this comment.
Constructing code strings by interpolating user-controlled data can lead to code injection vulnerabilities. Consider using a safer approach like passing data through stdin or a temporary file, or using JSON serialization instead of building Ruby literals.
- Catch ExceptionForMatrix::ErrDimensionMismatch in addition to Math::DomainError - Use diverse document vocabulary to reduce SVD instability - Reduce default benchmark sizes (5, 10, 15, 20) for native Ruby stability - Update test queries to match new document style
- Replace % string formatting with format() for readability - Fix code injection vulnerability by using JSON via stdin - Use Bundler.with_unbundled_env so subprocess can access GSL gem - Fix GSL::Vector missing count method (use to_a.count instead) - Update README with Ruby 3.4+ GSL installation instructions - Add real benchmark results showing 4x-116x speedup with GSL The benchmark comparison now works correctly, showing GSL provides significant speedups especially for build_index (SVD computation).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| puts format("%-20s %10s %10s %10.4f", 'TOTAL', '', '', total) | ||
|
|
||
| { results: results, gsl: Classifier::LSI.gsl_available, success: true } | ||
| rescue Math::DomainError, ExceptionForMatrix::ErrDimensionMismatch => e |
There was a problem hiding this comment.
The exception class ExceptionForMatrix::ErrDimensionMismatch appears to be incorrect. Ruby's Matrix library uses Matrix::ErrDimensionMismatch (without 'Exception' prefix). This rescue clause will not catch dimension mismatch errors as intended.
|
|
||
| # Use Bundler.with_unbundled_env if available to access system gems (like GSL) | ||
| popen_block = lambda do | ||
| IO.popen([env, RbConfig.ruby, '-I', lib_path, '-W0', '-e', <<~'RUBY'], 'r+') |
There was a problem hiding this comment.
The -W0 flag suppresses all warnings, which could hide important runtime issues during benchmarking. Consider using -W1 (default warnings) or removing this flag to ensure warnings about potential problems are visible.
| result = run_subprocess(docs_json, NATIVE_VECTOR: 'true', SUPPRESS_GSL_WARNING: 'true') | ||
|
|
||
| if result[:error] | ||
| puts " #{size} docs: FAILED (#{result[:error].class.name.split('::').last})" |
There was a problem hiding this comment.
The error message formatting logic result[:error].class.name.split('::').last is duplicated. Extract this into a helper method or variable to improve maintainability.
| result = run_subprocess(docs_json, SUPPRESS_GSL_WARNING: 'true') | ||
|
|
||
| if result[:error] | ||
| puts " #{size} docs: FAILED (#{result[:error].class.name.split('::').last})" |
There was a problem hiding this comment.
The error message formatting logic result[:error].class.name.split('::').last is duplicated. Extract this into a helper method or variable to improve maintainability.
Summary
benchmark/lsi_benchmark.rbscript for performance testingrake benchmarktask to run with current configurationrake benchmark:comparetask to compare GSL vs native RubyREADME.md(renamed from.markdown)Usage
Benchmark Results (Native Ruby)
Benchmark Operations
add_items- Adding documents to the indexbuild_index- Building the SVD index (dominates total time)classify- Classification (100 iterations)search- Search queries (100 iterations)find_related- Finding related documents (100 iterations)Test Plan
rake benchmarkruns successfullyFixes #75