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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 41 additions & 4 deletions src/run.js
Expand Up @@ -485,9 +485,13 @@ const minimalcss = async options => {
);
}

// 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

stereobooster marked this conversation as resolved.
Show resolved Hide resolved

// Now, let's loop over ALL links and process their ASTs compared to
// the DOMs.
const decisionsCache = {};
const isSelectorMatchToAnyElement = selectorString => {
// Here's the crucial part. Decide whether to keep the selector
// Find at least 1 DOM that contains an object that matches
Expand Down Expand Up @@ -519,7 +523,6 @@ const minimalcss = async options => {
return;
}
const ast = stylesheetAsts[href];

csstree.walk(ast, {
visit: 'Rule',
enter: function(node, item, list) {
Expand Down Expand Up @@ -548,6 +551,40 @@ const minimalcss = async options => {
const selectorString = utils.reduceCSSSelector(
csstree.generate(node)
);

// Before we begin, do a little warmup of the decision cache.
// From a given selector, e.g. `div.foo p.bar`, we can first look
// up if there's an point by first doing a lookup for `div.foo`
// because if that doesn't exist we *know* we can ignore more
// "deeper" selectors like `div.foo p.bar` and `div.foo span a`.
const parentSelectors = utils.getParentSelectors(selectorString);

// If "selectorString" was `.foo .bar span`, then
// this `parentSelectors` array will be
// `['.foo', '.foo .bar']`.
// If `selectorString` was just `.foo`, then
// this `parentSelectors` array will be `[]`.
let bother = true;
parentSelectors.forEach(selectorParentString => {
if (bother) {
// Is it NOT in the decision cache?
if (selectorParentString in decisionsCache === false) {
decisionsCache[
selectorParentString
] = isSelectorMatchToAnyElement(selectorParentString);
}
// What was the outcome of that? And if the outcome was
// that it was NOT there, set the 'bother' to false which
// will popoulate the decision cache immediately.
if (!decisionsCache[selectorParentString]) {
bother = false;
decisionsCache[selectorString] = false;
}
} else {
decisionsCache[selectorParentString] = false;
}
});

if (selectorString in decisionsCache === false) {
decisionsCache[selectorString] = isSelectorMatchToAnyElement(
selectorString
Expand Down Expand Up @@ -612,9 +649,9 @@ const minimalcss = async options => {
// it too.
csso.compress(allCombinedAst, cssoOptions);
postProcessOptimize(allCombinedAst);

const finalCss = csstree.generate(allCombinedAst);
const returned = {
finalCss: csstree.generate(allCombinedAst),
finalCss,
stylesheetContents
};
return Promise.resolve(returned);
Expand Down
61 changes: 56 additions & 5 deletions src/utils.js
@@ -1,3 +1,19 @@
const csstree = require('css-tree');

/**
* Simple single argument memoize function. Only works if the first and
* only argument is a hashable. E.g. a string.
*/
function memoize(fn) {
const cache = {};
stereobooster marked this conversation as resolved.
Show resolved Hide resolved
return argument => {
if (argument in cache === false) {
cache[argument] = fn(argument);
}
return cache[argument];
};
}

/**
* Reduce a CSS selector to be without any pseudo class parts.
* For example, from `a:hover` return `a`. And from `input::-moz-focus-inner`
Expand All @@ -10,25 +26,25 @@
* @param {string} selector
* @return {string}
*/
const reduceCSSSelector = selector => {
const reduceCSSSelector = memoize(selector => {
return selector.split(
/:(?=([^"'\\]*(\\.|["']([^"'\\]*\\.)*[^"'\\]*['"]))*[^"']*$)/g
)[0];
};
});

/**
* Remove the ' and/or " at the beginning and end of a string if it has it.
* @param {string} string
* @return {string}
*/
const unquoteString = string => {
const unquoteString = memoize(string => {
const first = string.charAt(0);
const last = string.charAt(string.length - 1);
if (first === last && (first === '"' || first === "'")) {
return string.substring(1, string.length - 1);
}
return string;
};
});

/**
* Removes all sequences of two-or-more semicolons separated by zero-or-more
Expand All @@ -43,4 +59,39 @@ const removeSequentialSemis = css => {
return css;
};

module.exports = { reduceCSSSelector, removeSequentialSemis, unquoteString };
/**
* Given a string CSS selector (e.g. '.foo .bar .baz') return it with the
* last piece (split by whitespace) omitted (e.g. '.foo .bar').
* If there is no parent, return an empty string.
*
* @param {string} selector
* @return {string[]}
*/
function getParentSelectors(selector) {
if (!selector) return [];
const parentSelectors = [];
const selectorAst = csstree.parse(selector, { context: 'selector' });

let generatedCSS;
while (selectorAst.children.tail) {
selectorAst.children.prevUntil(
selectorAst.children.tail,
(node, item, list) => {
list.remove(item);
return node.type === 'Combinator' || node.type === 'WhiteSpace';
}
);
generatedCSS = csstree.generate(selectorAst);
if (generatedCSS) {
parentSelectors.push(generatedCSS);
}
}
return parentSelectors.reverse();
}

module.exports = {
reduceCSSSelector,
removeSequentialSemis,
unquoteString,
getParentSelectors
};
14 changes: 14 additions & 0 deletions tests/examples/nested-selectors.css
@@ -0,0 +1,14 @@
#foo
.link, #foo p {
color: red;
}

.one {
color: red;
}
.one .two {
color: orange;
}
.one .two .three {
color: green;
}
13 changes: 13 additions & 0 deletions tests/examples/nested-selectors.html
@@ -0,0 +1,13 @@

<!doctype html>
<html lang="en">
<head>
<meta charset="utf-8">
<link href="nested-selectors.css" rel="stylesheet">
</head>
<body>
<div id="foo">
<p>Text</p>
</div>
</body>
</html>
5 changes: 5 additions & 0 deletions tests/main.test.js
Expand Up @@ -163,6 +163,11 @@ test('form elements', async () => {
expect(finalCss).toMatch('option:selected');
});

test('nested selectors and domLookupsTotal', async () => {
const { finalCss } = await runMinimalcss('nested-selectors');
expect(finalCss).toMatch('#foo p{color:red}');
});

test('invalid css', async () => {
expect.assertions(1);

Expand Down
28 changes: 28 additions & 0 deletions tests/utils.test.js
Expand Up @@ -31,3 +31,31 @@ test('Test removeSequentialSemis', () => {
// multiple semicolon sequences
expect(f('a;b;;c;;;d;;;;')).toEqual('a;b;c;d;');
});

test('Test parentSelectors', async () => {
const f = utils.getParentSelectors;
// Simplest possible
expect(f('.foo .bar')).toEqual(['.foo']);
// Slightly less simple
expect(f('.foo .bar .baz')).toEqual(['.foo', '.foo .bar']);
// Empty array
expect(f('.foo')).toEqual([]);
// Less trivial
expect(f('.ui.dropdown>.dropdown.icon:before')).toEqual(['.ui.dropdown']);
expect(f('.ui.vertical.menu .dropdown.item>.dropdown.icon:before')).toEqual([
'.ui.vertical.menu',
'.ui.vertical.menu .dropdown.item'
]);
expect(
f(
'.ui.search.selection>.icon.input:not([class*="left icon"])>.icon~.remove.icon'
)
).toEqual([
'.ui.search.selection',
'.ui.search.selection>.icon.input:not([class*="left icon"])',
'.ui.search.selection>.icon.input:not([class*="left icon"])>.icon'
]);
expect(f('.ui[class*="right aligned"].search>.results')).toEqual([
'.ui[class*="right aligned"].search'
]);
});