GeezerVison is showing github.com — Make SecureSecurityPolicyConfig significantly faster by jhawthorn · Pull Request #506 · github/secure_headers
Conversation
jhawthorn
commented
Apr 20, 2023
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
oreoshake and KyFaSt reacted with heart emoji
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.
KyFaSt
left a comment
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
KyFaSt
KyFaSt approved these changes
+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|