Skip to content

Conversation

bmarkons
Copy link
Contributor

@bmarkons bmarkons commented May 18, 2017

New feature: Compare graphs for two different benchmarks.

Take a closer look at discussion.

@bmarkons bmarkons force-pushed the compare-across-benchmarks branch 30 times, most recently from d428644 to f268109 Compare May 25, 2017 12:24
@bmarkons bmarkons force-pushed the compare-across-benchmarks branch from 7e927ab to d342f82 Compare June 5, 2017 13:42
Copy link
Member

@robin850 robin850 left a comment

Choose a reason for hiding this comment

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

Nice work Marko ! 👏

var benchmarkRunDisplayCount = $('#benchmark_run_display_count').val();
var compareWithBenchmark = $('#benchmark_run_compare_with').val();

if(compareWithBenchmark){
Copy link
Member

Choose a reason for hiding this comment

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

Pretty nit-picky but maybe you could add surrounding spaces around braces.

switch(e.which) {
case 37: //left
optionIndex = incrementOptionIndex(optionIndex, optsLength, -1);
break;
Copy link
Member

Choose a reason for hiding this comment

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

Also nit-picky but it seems more idiomatic to align the break with the block's content in JavaScript.

@categories << version if version != @categories.last
if @comparing_runs.present?
(@benchmark_runs + @comparing_runs)
.sort_by{ |benchmark_run| benchmark_run.initiator.created_at }
Copy link
Member

Choose a reason for hiding this comment

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

As @tgxworld suggested earlier, you could indent this by a level.

Also, to be honest I didn't test it but there should be a way to make this method a little bit easier to read. What do you think about something like:

def build_columns
  columns = {}

  if @comparing_runs.present?
    runs = (@benchmark_runs + @comparing_runs)
             .sort_by { |run| run.initiator.created_at }
  else
    runs = @benchmark_runs
  end
    
  runs.each do |run|
    version = nil
    if block_given?
      version = yield(run)
      @categories ||= []
      @categories << version if version != @categories.last
    end

    run.result.each do |key, value|
      if @comparing_runs.present?
        columns["#{key}_#{run.benchmark_type.category}"] ||= []
        columns["#{key}_#{run.benchmark_type.category}"] << [version, value.to_f]
      else
        columns[key] ||= []
        columns[key] << value.to_f
      end
    end
  end

  @columns = columns.map do |name, data|
    { name: name, data: data}
  end

  self
end

@SamSaffron
Copy link
Member

@bmarkons @robin850 had some comments, can you be sure to address them and the merge conflict, I really want this merged into the repo and on the website as soon as possible.

We can refine it further in future PRs

@bmarkons
Copy link
Contributor Author

bmarkons commented Jun 6, 2017

sure @SamSaffron, I'm on it.

@bmarkons bmarkons force-pushed the compare-across-benchmarks branch from a01f6ba to a1ee0af Compare June 6, 2017 19:12
@bmarkons bmarkons force-pushed the compare-across-benchmarks branch from 57b992e to 3384478 Compare June 6, 2017 20:44
@bmarkons bmarkons force-pushed the compare-across-benchmarks branch from 3384478 to 7670006 Compare June 6, 2017 20:51
@bmarkons
Copy link
Contributor Author

bmarkons commented Jun 6, 2017

Thanks @robin850 :) I've made changes you've suggested.

@robin850
Copy link
Member

robin850 commented Jun 6, 2017

@bmarkons : No problem, good job. Does the timeout change is related to the refactoring of the build_columns method ? The code I've given may be terribly inefficient.

@bmarkons
Copy link
Contributor Author

bmarkons commented Jun 6, 2017

@robin850 : No, that change doesn't affect performance. The code I've written in this PR is pretty much inefficient but I plan to make it better in next iteration.

@robin850
Copy link
Member

robin850 commented Jun 6, 2017

Okay great, this can be shipped then ! 🙂 🎉

@tgxworld tgxworld merged commit 92a962f into ruby-bench:master Jun 7, 2017
@bmarkons bmarkons deleted the compare-across-benchmarks branch June 7, 2017 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants