Skip to content

improve performance (slightly) #118

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Aug 23, 2021
Merged

improve performance (slightly) #118

merged 2 commits into from
Aug 23, 2021

Conversation

flavorjones
Copy link
Member

This PR will make one less method call per element in a document.

Also rewrite the test coverage for PIs and comments from ab16fa4 to test behavior and not implementation.

@flavorjones
Copy link
Member Author

flavorjones commented Aug 20, 2021 •
edited
Loading

Benchmark results running on a ~70kb real-world HTML document (https://github.com/sparklemotion/nokogiri/blob/main/test/files/tlm.html):

Warming up --------------------------------------
               1.4.1     1.000  i/100ms
            proposed     1.000  i/100ms
Calculating -------------------------------------
               1.4.1     13.480  (±29.7%) i/s -     64.000  in   5.036261s
            proposed     13.893  (±28.8%) i/s -     66.000  in   5.021529s

Comparison:
            proposed:       13.9 i/s
               1.4.1:       13.5 i/s - same-ish: difference falls within error

@flavorjones flavorjones force-pushed the flavorjones-tweak-comment-and-pi branch from 51e75dc to 692eb8a Compare August 20, 2021 19:51
@flavorjones
test: rewrite test coverage for comments and PIs
ab21d78 
to test behavior and not implementation.
@flavorjones
perf: PermitScrubber#scrub checks node.element? before node.comment?
b1fe437 
Assuming elements are more common than comments, this is going to be
one less method call per node.
@flavorjones flavorjones force-pushed the flavorjones-tweak-comment-and-pi branch from 692eb8a to b1fe437 Compare August 20, 2021 19:53
kaspth
kaspth approved these changes Aug 23, 2021
View reviewed changes
Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

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

Good call 👍

@flavorjones flavorjones merged commit d494309 into master Aug 23, 2021
@flavorjones flavorjones deleted the flavorjones-tweak-comment-and-pi branch August 23, 2021 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rafaelfranca rafaelfranca Awaiting requested review from rafaelfranca

1 more reviewer

@kaspth kaspth kaspth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@flavorjones @kaspth