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

publisher: Exclude comment and doctype elements from writeStats #8443

Conversation

dirkolbrich
Copy link
Contributor

  • Reorder code blocks
  • Rename cssClassCollectorWriter to htmlElementCollectorWriter, as it just collect html element information
  • Expand benchmark to test for minified and unminified content

Fixes #8396, Fixes #8417

I somehow messed up the code review and my following additions with squashing theses commits into the original pull request. Thats why this PR is reopened.

With the original bench test, this PR compares:

➜  publisher git:(master) benchstat ~/old.txt ~/new.txt 
name                    old time/op    new time/op    delta
ClassCollectorWriter-8    24.4µs ± 1%    23.9µs ± 1%   -2.03%  (p=0.000 n=10+9)

name                    old alloc/op   new alloc/op   delta
ClassCollectorWriter-8    35.0kB ± 0%    34.4kB ± 0%   -1.83%  (p=0.000 n=10+10)

name                    old allocs/op  new allocs/op  delta
ClassCollectorWriter-8       155 ± 0%       139 ± 0%  -10.32%  (p=0.000 n=10+10)

When the bench tests on master are expanded to the new version (see branch htmlcollector-bench), the comparison is even more favorable:

➜  publisher git:(master) benchstat ~/old2.txt ~/new.txt
name                                       old time/op    new time/op    delta
ClassCollectorWriter-8                       24.5µs ± 1%    23.9µs ± 1%   -2.53%  (p=0.000 n=9+9)
ElementsCollectorWriter-8                    71.7µs ± 1%    68.2µs ± 2%   -4.94%  (p=0.000 n=10+10)
ElementsCollectorWriterMinified-8            71.6µs ± 0%    68.0µs ± 0%   -5.13%  (p=0.000 n=9+10)
ElementsCollectorWriterWithMinifyStream-8    96.9µs ± 1%    94.0µs ± 1%   -3.04%  (p=0.000 n=10+9)
ElementsCollectorWriterWithMinifyString-8    97.0µs ± 0%    93.4µs ± 1%   -3.76%  (p=0.000 n=10+10)

name                                       old alloc/op   new alloc/op   delta
ClassCollectorWriter-8                       35.0kB ± 0%    34.4kB ± 0%   -1.82%  (p=0.000 n=10+10)
ElementsCollectorWriter-8                     109kB ± 0%     102kB ± 0%   -6.33%  (p=0.000 n=10+9)
ElementsCollectorWriterMinified-8             110kB ± 0%     103kB ± 0%   -6.28%  (p=0.000 n=9+10)
ElementsCollectorWriterWithMinifyStream-8     112kB ± 0%     105kB ± 0%   -6.22%  (p=0.000 n=10+10)
ElementsCollectorWriterWithMinifyString-8     115kB ± 0%     108kB ± 0%   -5.97%  (p=0.000 n=10+10)

name                                       old allocs/op  new allocs/op  delta
ClassCollectorWriter-8                          155 ± 0%       139 ± 0%  -10.32%  (p=0.000 n=10+10)
ElementsCollectorWriter-8                       418 ± 0%       350 ± 0%  -16.27%  (p=0.000 n=10+10)
ElementsCollectorWriterMinified-8               419 ± 0%       352 ± 0%  -15.99%  (p=0.000 n=10+10)
ElementsCollectorWriterWithMinifyStream-8       442 ± 0%       375 ± 0%  -15.16%  (p=0.000 n=10+10)
ElementsCollectorWriterWithMinifyString-8       450 ± 0%       383 ± 0%  -14.89%  (p=0.000 n=10+10)

- Reorder code blocks
- Rename cssClassCollectorWriter to htmlElementCollectorWriter, as it just collect html element information
- Expand benchmark to test for minified and unminified content

Fixes gohugoio#8396, Fixes gohugoio#8417
@dirkolbrich
Copy link
Contributor Author

#8431

@bep
Copy link
Member

bep commented Apr 20, 2021

I made some minor tweaks and merged this manually, great job, much appreciated!

@bep bep closed this Apr 20, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants