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

Make SecureSecurityPolicyConfig significantly faster #506

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Apr 20, 2023

We have been seeing this gem a lot in profiles. Must of this slowness seems to come from overuse of instance variables in DynamicConfig and attempting to use them basically as a hash (which we can do much faster with a hash 😅)

The first commit of these is the most important, but the other two also significantly speed things up.

There is definitely more improvement available here, we seem to be overly cautious in duplicating arrays, and we also seem to convert unnecessarily between hashes and the config object, but I think this is the best place to start.

Benchmark:
require "secure_headers"
require "benchmark/ips"

# Copied from README
MyCSPConfig = SecureHeaders::ContentSecurityPolicyConfig.new(
  preserve_schemes: true, # default: false. Schemes are removed from host sources to save bytes and discourage mixed content.
  disable_nonce_backwards_compatibility: true, # default: false. If false, `unsafe-inline` will be added automatically when using nonces. If true, it won't. See #403 for why you'd want this.

  # directive values: these values will directly translate into source directives
  default_src: %w('none'),
  base_uri: %w('self'),
  block_all_mixed_content: true, # see https://www.w3.org/TR/mixed-content/
  child_src: %w('self'), # if child-src isn't supported, the value for frame-src will be set.
  connect_src: %w(wss:),
  font_src: %w('self' data:),
  form_action: %w('self' github.com),
  frame_ancestors: %w('none'),
  img_src: %w(mycdn.com data:),
  manifest_src: %w('self'),
  media_src: %w(utoob.com),
  object_src: %w('self'),
  sandbox: true, # true and [] will set a maximally restrictive setting
  plugin_types: %w(application/x-shockwave-flash),
  script_src: %w('self'),
  script_src_elem: %w('self'),
  script_src_attr: %w('self'),
  style_src: %w('unsafe-inline'),
  style_src_elem: %w('unsafe-inline'),
  style_src_attr: %w('unsafe-inline'),
  worker_src: %w('self'),
  upgrade_insecure_requests: true, # see https://www.w3.org/TR/upgrade-insecure-requests/
  report_uri: %w(https://report-uri.io/example-csp)
)


Benchmark.ips do |x|
  x.report "csp_config.to_h" do
    MyCSPConfig.to_h
  end

  x.report "csp_config.append" do
    MyCSPConfig.append({})
  end

  x.report "new(config).value" do
    SecureHeaders::ContentSecurityPolicy.new(MyCSPConfig).value
  end
end

Before:

$ be ruby bench.rb
Warming up --------------------------------------
     csp_config.to_h    13.737k i/100ms
   csp_config.append     2.105k i/100ms
   new(config).value     1.429k i/100ms
Calculating -------------------------------------
     csp_config.to_h    139.988k (± 0.3%) i/s -    700.587k in   5.004666s
   csp_config.append     21.133k (± 2.4%) i/s -    107.355k in   5.082856s
   new(config).value     14.298k (± 0.4%) i/s -     72.879k in   5.097116s

After:

$ be ruby bench.rb
Warming up --------------------------------------
     csp_config.to_h   123.784k i/100ms
   csp_config.append     4.181k i/100ms
   new(config).value     1.617k i/100ms
Calculating -------------------------------------
     csp_config.to_h      1.238M (± 3.1%) i/s -      6.189M in   5.003769s
   csp_config.append     40.921k (± 1.0%) i/s -    204.869k in   5.006924s
   new(config).value     16.095k (± 0.4%) i/s -     80.850k in   5.023259s

to_h is 10x faster, append is 2x faster, and .value (which was not the target of these optimizations but I didn't want to see it regress) is slightly faster

Previously this class had a large number of instance variables being
used as storage, however they were almost entirely being accessed in a
hash-like way using send and instance_variable_set, both of which are
slower than just using a hash.

By just using a hash we make this simpler and faster. We can also keep
the hash sparse rather than using `nil` as a "not set" sentinel value.
These were only used in one place. We can avoid complexity by not
defining them.
For some reason Rubocop doesn't like the case equality operator (it's
good) so we can just use a case statement I guess.
Copy link
Contributor

@KyFaSt KyFaSt left a comment

Choose a reason for hiding this comment

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

Hi @jhawthorn, thanks so much for this PR. We were able to test and verify this change in the github project, feel free to merge. We have another change we'd like to add so you don't need to cut a release, our team will cut the release once the change is added. In favor of making the smallest possible change, our team will cut a separate release for this PR once you have merged the PR.

@KyFaSt KyFaSt self-requested a review July 10, 2023 21:33
@KyFaSt KyFaSt merged commit 7a23cb6 into github:main Aug 11, 2023
5 checks passed
Copy link

@Ninja-mann07 Ninja-mann07 left a comment

Choose a reason for hiding this comment

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

Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 11, 2024
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 12, 2024
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 14, 2024
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 15, 2024
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 15, 2024
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 15, 2024
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 16, 2024
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 17, 2024
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 17, 2024
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 17, 2024
Eric-Guo added a commit to Eric-Guo/openproject that referenced this pull request Oct 17, 2024
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.

3 participants