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

Rewrite `script/rubyprof` as a Ruby script #6813

Merged
merged 2 commits into from Mar 1, 2018

Conversation

Projects
None yet
5 participants
@ashmaroli
Member

ashmaroli commented Feb 28, 2018

The original script seemed to be unused as it has been left broken for a long time..
(The script was testing Jekyll::Commands::Build.process({'source' => 'site'}))

..and there were issues while trying to get it to run on Windows , even after the source was corrected to the current docs site at ./docs (NameError: uninitialized constant CallTreePrinter)

So, I rewrote it as a Ruby script.


Usage changes

  1. Invocation:
- $ bash script/rubyprof
+ $ bundle exec ruby script/rubyprof
  1. The generated result is now a simpler text output via RubyProf::FlatPrinter instead of RubyProf::CallTreePrinter earlier.
  2. The generated filename now has a time-stamp as against a random integer earlier

@DirtyF DirtyF requested a review from jekyll/core Feb 28, 2018

@DirtyF DirtyF added the internal label Feb 28, 2018

@parkr

parkr approved these changes Feb 28, 2018

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 28, 2018

Example output:

Measure Mode: wall_time
Thread ID: 70232756713420
Fiber ID: 70232762760360
Total: 27.370537
Sort by: self_time

 %self      total      self      wait     child     calls  name
  4.08      1.345     1.118     0.000     0.227   111111   Nokogiri::XML::Node#native_write_to
  3.36      0.924     0.920     0.000     0.004      441   <Class::Nokogiri::HTML::Document>#read_memory
  1.79      3.458     0.491     0.000     2.967   111111   Nokogiri::XML::Node#serialize
  1.64      3.906     0.449     0.000     3.458   249818  *Jekyll::Drops::Drop#[]
  1.63      2.342     0.445     0.000     1.897   111111   Nokogiri::XML::Node#write_to
  1.45      0.398     0.398     0.000     0.000   295944   Kernel#methods
  1.43      0.394     0.392     0.000     0.002      326   Nokogiri::XML::Node#in_context
  1.38      4.557     0.379     0.000     4.178   111111   Nokogiri::XML::Node#to_format
  1.38      0.453     0.378     0.000     0.075      168  *Kernel#load
  1.32      5.841     0.362     0.000     5.479   506985  *Class#new
  1.31      3.635     0.358     0.000     3.277    12787  *Array#select
  1.24      0.339     0.339     0.000     0.000    79279   String#=~
  1.22      0.334     0.334     0.000     0.000   305775   String#split
  1.17      0.337     0.321     0.000     0.016     8276   Nokogiri::XML::Node#add_child_node
  1.17      0.416     0.320     0.000     0.096     5136   Array#uniq
  1.06      2.364     0.291     0.000     2.073   180460   Jekyll::Filters#item_property
  1.06      0.290     0.290     0.000     0.000  1124383   Kernel#is_a?
....
@DirtyF

DirtyF approved these changes Feb 28, 2018

@DirtyF DirtyF requested a review from jekyll/performance Feb 28, 2018

Gemfile Outdated
if ENV["BENCHMARK"]
gem "benchmark-ips"
gem "rbtrace"
gem "ruby-prof"

This comment has been minimized.

@pathawks

pathawks Feb 28, 2018

Member

What's this about?

This comment has been minimized.

@ashmaroli

ashmaroli Feb 28, 2018

Member

so that one doesn't have to run

BENCHMARK=1 script/rubyprof

or

BENCHMARK=1 bundle exec ruby script/rubyprof

This comment has been minimized.

@pathawks

pathawks Feb 28, 2018

Member

So, should we just get rid of the if ENV["BENCHMARK"] guard altogether?

This comment has been minimized.

@ashmaroli

ashmaroli Feb 28, 2018

Member

no I prefer having that guard because, stackprof cannot be installed on Windows.. and this guard ensures it's not looked for by Bundler

This comment has been minimized.

@pathawks

pathawks Feb 28, 2018

Member

So change the guard to look for Windows?

This comment has been minimized.

@ashmaroli

ashmaroli Mar 1, 2018

Member

That would be a nice alternative, but I do not want to change all that in this PR.
This PR is dealing with just script/rubyprof that seemed to have been left unused.., so, changes to it wouldn't disrupt an existing workflow..

This comment has been minimized.

@pathawks

pathawks Mar 1, 2018

Member

Let’s revert this and discuss it in a separate PR then?

This comment has been minimized.

@pathawks

pathawks Mar 1, 2018

Member

I’m not trying to start a war. I’m just trying to understand the motivation for the change.

This comment has been minimized.

@ashmaroli

ashmaroli Mar 1, 2018

Member

okay.. I'll concede and revert this change for a future discussion..

@DirtyF

This comment has been minimized.

Member

DirtyF commented Mar 1, 2018

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 4032c3e into jekyll:master Mar 1, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ashmaroli ashmaroli deleted the ashmaroli:rubyprof-script branch Mar 1, 2018

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