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

Optimize DOM selector lookups by pre-warming by selectors' parents #296

Merged
merged 3 commits into from
Feb 5, 2019

Conversation

peterbe
Copy link
Owner

@peterbe peterbe commented Feb 4, 2019

Part of #287

@peterbe
Copy link
Owner Author

peterbe commented Feb 4, 2019

I used this page to test things: https://www.peterbe.com/plog-original.html

It uses Semantic-UI which is horribly verbose! (Hint view source and look at the CSS)
Because this patch introduces some changes to how it prepopulates the decision cache with some dead-obvious selectors, the output is different compared to master. Diff:

183c183,185
< * {
---
> *,
> :after,
> :before {
196,197d197
<   height: 100%;
<   font-size: 14px;
203,206d202
< body,
< h1 {
<   padding: 0;
< }
208a205,206
>   margin: 0.67em 0;
>   line-height: 1.28571429em;
210a209
>   padding: 0;
213,214d211
<   margin: 0.67em 0;
<   line-height: 1.28571429em;
231c228,233
< body {
---
> ::-webkit-file-upload-button {
>   -webkit-appearance: button;
>   font: inherit;
> }
> body,
> html {
232a235,239
> }
> html {
>   font-size: 14px;
> }
> body {
233a241
>   padding: 0;
259a268,279
> ::-webkit-selection {
>   background-color: #cce2ff;
>   color: rgba(0, 0, 0, 0.87);
> }
> ::-moz-selection {
>   background-color: #cce2ff;
>   color: rgba(0, 0, 0, 0.87);
> }
> ::selection {
>   background-color: #cce2ff;
>   color: rgba(0, 0, 0, 0.87);
> }

(red means output with master and green output with this branch)

Here's the master.css and branch.css outputs prettierfied.

I'm pretty sure this is correct fix. Things like Semantic-UI does like...

*, :before, :after {
  -webkit-box-sizing:inherit;box-sizing:inherit
}

should not be deleted from the final CSS. (not that I understand the CSS itself)

@peterbe
Copy link
Owner Author

peterbe commented Feb 4, 2019

In terms of speed...

Before:

▶ run-forever.sh time node bin/minimalcss.js https://www.peterbe.com/plog-original.html -o /dev/null
To stop this forever loop created a file called stopnow.
E.g: touch stopnow

Now going to run 'time node bin/minimalcss.js https://www.peterbe.com/plog-original.html -o /dev/null' forever (or until stopnow exists)


real	0m6.837s
user	0m3.733s
sys	0m0.357s

real	0m6.728s
user	0m4.254s
sys	0m0.273s

real	0m6.713s
user	0m4.225s
sys	0m0.295s

real	0m6.175s
user	0m3.726s
sys	0m0.272s

real	0m6.206s
user	0m3.785s
sys	0m0.263s

real	0m6.341s
user	0m3.906s
sys	0m0.258s

real	0m6.187s
user	0m3.735s
sys	0m0.265s

real	0m6.165s
user	0m3.730s
sys	0m0.251s

After:

▶ run-forever.sh time node bin/minimalcss.js https://www.peterbe.com/plog-original.html -o /dev/null
To stop this forever loop created a file called stopnow.
E.g: touch stopnow

Now going to run 'time node bin/minimalcss.js https://www.peterbe.com/plog-original.html -o /dev/null' forever (or until stopnow exists)


real	0m4.941s
user	0m2.470s
sys	0m0.239s

real	0m5.020s
user	0m2.521s
sys	0m0.234s

real	0m4.984s
user	0m2.564s
sys	0m0.231s

real	0m5.302s
user	0m2.787s
sys	0m0.244s

real	0m4.888s
user	0m2.454s
sys	0m0.233s

real	0m5.210s
user	0m2.481s
sys	0m0.233s

real	0m5.161s
user	0m2.769s
sys	0m0.248s

real	0m5.282s
user	0m2.841s
sys	0m0.248s

real	0m5.013s
user	0m2.609s
sys	0m0.238s

real	0m5.068s
user	0m2.671s
sys	0m0.248s

real	0m5.113s
user	0m2.664s
sys	0m0.252s

That means, on median the total processing time goes from 6.3s to 5.1s.

@peterbe
Copy link
Owner Author

peterbe commented Feb 4, 2019

@stereobooster This one is quite complex but I think it's valuable. Not expecting a deep code review but if you could take a look I'd appreciate it. Thanks in advance.

src/utils.js Outdated Show resolved Hide resolved
// This is the protection against ever looking up the same CSS selector
// more than once. We can confidently pre-populate it with a couple that
// we're confident about.
const decisionsCache = { '*': true, body: true, html: true, '': true };
Copy link
Collaborator

Choose a reason for hiding this comment

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

What empty string selector '' stands for?

Copy link
Owner Author

Choose a reason for hiding this comment

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

You know, I'm actually not sure.

If you look at view-source:https://www.peterbe.com/static/css/base.min.79787297dbf5.css (which comes from https://github.com/peterbe/django-peterbecom/blob/master/peterbecom/base/static/css/semantic/reset.min.css) you see this:

*,:after,:before{-webkit-box-sizing:inherit;box-sizing:inherit}

And if you remove the pseudo stuff all you're left with is ''.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess there is an error where we convert AST to selectors, it should be :after instead of ``. Or at least we can add comment

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think it comes from reduceCSSSelector. In terms of CSS selector and DOM inspecting the thing to look for is before the :. But if the whole CSS selector is :after { ... } then his is what happens.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Add small explanation why there is empty string, so we don't forget

Copy link
Owner Author

Choose a reason for hiding this comment

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

Bummer! I forgot this before I merged.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done d16f2c1

src/run.js Outdated Show resolved Hide resolved
src/run.js Outdated Show resolved Hide resolved
tests/utils.test.js Outdated Show resolved Hide resolved
Co-Authored-By: peterbe <peterbe@mozilla.com>
Copy link
Collaborator

@stereobooster stereobooster left a comment

Choose a reason for hiding this comment

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

🥇

@peterbe peterbe merged commit e62652c into master Feb 5, 2019
@peterbe peterbe deleted the 287-slow-when-html-+-css-is-large branch February 5, 2019 16:33
@peterbe
Copy link
Owner Author

peterbe commented Feb 5, 2019

Thank you @stereobooster !

It still saddens me that it takes ~5 seconds to execute against these monster CSS files. The blame lies with Semantic-UI for being so overly verbose in its style.

@peterbe
Copy link
Owner Author

peterbe commented Feb 5, 2019

By the way, for https://www.peterbe.com/plog-original.html prior to this branch there had to be 3,285 calls to dom(selectorString).length > 0. As of this patch there now needs only to be 1,563 calls.

So 3,285 cheerio DOM lookups to 1,563 essentially reduced the total lookup time from ~6s to ~5s.

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.

2 participants