Skip to content
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

3.x perf #506

Closed
wants to merge 8 commits into from
Closed

3.x perf #506

wants to merge 8 commits into from

Conversation

janbiedermann
Copy link

@janbiedermann janbiedermann commented Oct 14, 2017

numbers without cache adjustment

3.x
tests:
Finished in 29.491941s, 27.0582 runs/s, 116.6081 assertions/s.
798 runs, 3439 assertions, 0 failures, 0 errors, 2 skips

first page get:
o_jit:   0.520000   0.080000   0.600000 (  0.598037)
o_jit:   3.860000   0.570000   4.430000 (  4.425943)
second page get:
o_jit:   0.400000   0.020000   0.420000 (  0.422710)
o_jit:   2.510000   0.090000   2.600000 (  2.602424)
recompile first page get:
o_jit:   0.420000   0.020000   0.440000 (  0.440247)
o_jit:  10.140000   2.850000  12.990000 ( 13.115074)
recompile second page get:
o_jit:   0.400000   0.020000   0.420000 (  0.419903)
o_jit:   9.200000   2.840000  12.040000 ( 12.052266)

combined asset:
o_jit_c_first:   4.970000   0.790000   5.760000 (  5.802336)
o_jit_c_second:   3.220000   0.110000   3.330000 (  3.345740)
o_jit_c_recomp_first:  12.120000   3.290000  15.410000 ( 15.490749)
o_jit_c_recomp_second: 11.320000   3.300000  14.620000 ( 14.694551)  

assets:precompile: real	2m55.506s
3.x_perf
tests:
798 runs, 3434 assertions, 0 failures, 0 errors, 2 skips
Finished in 28.709360s, 27.7958 runs/s, 119.6126 assertions/s.

first page get:
o_jit:   0.390000   0.070000   0.460000 (  0.463624)
o_jit:   3.020000   0.550000   3.570000 (  3.573947)
second page get:
o_jit:   0.280000   0.010000   0.290000 (  0.302562)
o_jit:   1.690000   0.060000   1.750000 (  1.750890)
recompile first page get:
o_jit:   0.250000   0.010000   0.260000 (  0.265351)
o_jit:   7.260000   1.580000   8.840000 (  8.831509)
recompile second page get:
o_jit:   0.270000   0.010000   0.280000 (  0.282441)
o_jit:   6.860000   1.550000   8.410000 (  8.408544)

combined asset:
o_jit_c_first:  3.030000   0.630000   3.660000 (  3.661370)
o_jit_c_second:   2.020000   0.070000   2.090000 (  2.113375)
o_jit_c_recomp_first:  8.800000   1.940000  10.740000 ( 10.784423)
o_jit_c_recomp_second:   8.300000   1.790000  10.090000 ( 10.112734)

assets:precompile: real	2m45.752s

o_jit: measured from within opals javascript_include_tag
recompile meaning recompiling an opal .rb file with sprockets
first ojit is an large static asset
second o_jit is the dynamic one for recompile

patches beside the regexp patch are for compiling performance.

tests run with matz ruby 2.4.2

#503

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rails-bot
Copy link

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against 3.x. Please double check that you specified the right target!

use default cache size for new caches
@janbiedermann
Copy link
Author

janbiedermann commented Oct 15, 2017

the cache size is the real deal here, i got CONSTANT cache misses, and even worse, sprockets limited the internal cache size to 1024 entries, so that was killing everything

i get unbelievable good numbers now, too good to publish :)

@@ -56,6 +56,7 @@ def parse_q_values(values)
#
# Returns Array of matched Strings from available Array or [].
def find_q_matches(q_values, available, &matcher)
return [] if available == []
Copy link
Member

Choose a reason for hiding this comment

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

We're allocating an array everytime this if code is called. We can speed it up by using a pre-allocated constant

require 'benchmark/ips'

x = ""

EMPTY_ARRAY = [].freeze

Benchmark.ips do |x|
  x.report("constant") { x if x == EMPTY_ARRAY }
  x.report("literal ") { x if x == [] }
end

Gives us

Warming up --------------------------------------
            constant   326.041k i/100ms
            literal    306.879k i/100ms
Calculating -------------------------------------
            constant     10.675M (± 7.9%) i/s -     53.145M in   5.011048s
            literal       8.301M (± 7.7%) i/s -     41.429M in   5.022438s

Comparison:
            constant: 10675185.0 i/s
            literal :  8300648.6 i/s - 1.29x  slower

@@ -35,6 +35,21 @@ def compress
scheme + compressed_path
end

def self.fast_compress(uri, root)
if uri.include?('://'.freeze)
Copy link
Member

Choose a reason for hiding this comment

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

We don't need all the freeze calls. We're using the magic comment in the files:

# frozen_string_literal: true

Copy link
Author

Choose a reason for hiding this comment

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

oh, right, missed that

@schneems
Copy link
Member

Sorry for the delay here. I think i'm ready to start looking at merging some of this stuff. Can you split this up into individual patches?

Don't convert double quotes to single, performance difference is negligible and it messes up the git blame. My personal preference is double quotes everywhere so when they're mixed and right next to each other it looks fine.

for the a_v_t_d we already talked about this a ton here's some info. Using case is slower than the current method

I'm not sure what risks there are by changing the cache default numbers.

@janbiedermann
Copy link
Author

well, problem are different ruby implementations and platforms and so on, its faster for me, but slower for others, i dont mind throwing a_v_t_d in the bin or the other minor improvements. A lot of speed is gained by other means. Most important here is the regexp patch.

@schneems
Copy link
Member

Are you on opal? Do you know if it implemented compare by identity? https://github.com/rails/sprockets/pull/439/files

@janbiedermann
Copy link
Author

oops, i missed your question, sorry. since 0.11 it has - Added Hash#compare_by_identity (#1657)
closing this, because i created a opal-webpack-loader

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants