Audit: DOM stats (total nodes, depth, width) #1673

ebidel · 2017-02-09T02:30:58Z

R: all

The audit is now scored according to the distribution linked to in the source.
Adds a "cards formatter" to present hero stat info. If we like the direction, there are some other stat candidates in the report we can move over. I played with hiding the cards behind a <details>, but it didn't feel right. It's kinda nice to see the cards to break up our walls of text. More grey, but they don't have to be...?!
Smoke tests, formatter tests, and audit tests. The whole shebang.

Here's what she looks like now:

As seen in the screenshot, there's a title attribute for the last two cards that gives users a selector snippet on mouse over. It give context of the user's DOM tree.

patrickhulce

Was the 1500 also found in one of those links? I couldn't find it. Maybe link to wherever he said that if not. Also would love to see your swaggy HTTP archive stats here :)

patrickhulce · 2017-02-09T16:55:45Z

lighthouse-core/gather/gatherers/dobetterweb/domnodes.js

+   * @return {!Promise<!Array<!Object>>}
+   */
+  afterPass(options) {
+    return options.driver.querySelectorAll('body, body /deep/ *');


yikes how big is this? Do we have plans to do anything with them or just use the length? Then again I've probably done far more burdensome things than this :)

It was in a comment I re-posted from an email thread:
#1096 (comment)

Would really love more hard data on the topic, but I do trust whatever Elliot says. I benchmarked a few big apps in the past and they tend to hover around ~1500-2000 nodes (GMail, maps, inbox, github).

Gmail is 4000-5000 elements (depending on the view, sometimes it's 15k on long email threads). This github review page is ~1500. My facebook timeline is 2300 when loaded. I got my numbers sampling a bunch of popular websites.

btw the /deep/ trick won't work with Shadow DOM v1 because I don't think deep will match in there. You'll need to write your own traversal that crawls the entire document. It's unfortunate we don't have any tooling for querySelectorAll with Shadow DOM v1 yet. @hayatoito

Here's the crbug: https://bugs.chromium.org/p/chromium/issues/detail?id=633007#c16

Looks like >>> in qSA is behind the experimental web platform flag. We can probably move to something like https://gist.github.com/ebidel/fc1302d5fa1ef7c6d42fe8189acc3820 to cover all base.

DevTools also has https://chromedevtools.github.io/debugger-protocol-viewer/tot/DOM/#method-getFlattenedDocument, which might be even better.

@patrickhulce we also have the same query here:

lighthouse/lighthouse-core/gather/gatherers/dobetterweb/all-event-listeners.js

Line 133 in c6aeb33

.then(_ => options.driver.querySelectorAll('body, body /deep/ *')) // drill into shadow trees

We could get trickier and create a shared driver method that returns a promise and caches the result after the first call. Thoughts?

agree with @patrickhulce that this may be a really big thing to bring back over and keep around just for the length. It doesn't look like the array contents would be particularly helpful for others audits, just an array of node IDs that would then each be wrapped by Elements, but we wouldn't be able to tell them apart in any way without more protocol requests for each one...

Particularly if you're going to have to get tricky with a selector for Shadow DOM v1, maybe switch to evaluateAsync and ship back only the number of nodes?

We could get trickier and create a shared driver method that returns a promise and caches the result after the first call. Thoughts?

We probably should measure, I guess :) Maybe it's not so bad.

Beyond perf, though, for this gatherer there doesn't seem to be any benefit to keeping the array around

Isn't Pavel adding a method specifically to stop us from doing that? :) Haha, if its just a few thousand node ids I'm not that concerned.

DOM.getFlattenedDocument? I did a quick dirty benchmark between the two and found querySelectorAll to be faster:

Runs: 1337ms, 1033ms

console.time('took'); return options.driver.sendCommand('DOM.enable') .then(_ => options.driver.sendCommand('DOM.getFlattenedDocument', { depth: -1, pierce: true })) .then(result => { const nodes = result.nodes.filter(node => node.nodeType === 1); console.timeEnd('took'); return nodes; });

Runs: 812ms, 425ms

console.time('took'); return options.driver.querySelectorAll('html, html /deep/ *') .then(nodes => { console.timeEnd('took'); return nodes; });

IMO, DOM.getFlattenedDocument is a bit clunky to use. You have to filter for element nodes, but the biggest drawback is that depth: -1, pierce: true also traverses into iframes. We definitely don't want that.

Yeah I'm fine with it here, just making a comment on why sharing the code may not be a great reason to justify fetching the nodes instead of length (if the other path that's using it will start using one that doesn't need to go and fetch information for each of the items individually after this call finishes :) )

patrickhulce · 2017-02-09T16:56:54Z

lighthouse-core/audits/dobetterweb/num-dom-nodes.js

+          `~${NumDOMNodes.MAX_DOM_NODES} DOM nodes. The sweet spot is around 60 elements wide x ` +
+          '32 elements deep. A large DOM can increase memory, cause longer ' +
+          '[style calculations](https://developers.google.com/web/fundamentals/performance/rendering/reduce-the-scope-and-complexity-of-style-calculations), ' +
+          'and produce costly [layout reflows](https://developers.google.com/speed/articles/reflow). [Learn more](hhttps://developers.google.com/web/fundamentals/performance/rendering/).',


s/hhttps/https/

ebidel · 2017-02-09T17:10:57Z

swaggy HTTP archive stats here

I'm selfishly adding this audit to have more places to show httparchive data :)

brendankenny · 2017-02-10T22:25:13Z

lighthouse-core/audits/dobetterweb/num-dom-nodes.js

+  static audit(artifacts) {
+    const nodes = artifacts.AllDOMNodes;
+
+    const rawValue = nodes.length <= NumDOMNodes.MAX_DOM_NODES;


it would be great to score these like we do the FMP/TTI/etc audits, so it isn't just "you passed this line and are now failing" and more like, "you are way out of the mainstream/what browsers can handle well, so you score a 25" or whatever.

It would also be nice to be able to quantify the harm done by the amount of DOM content, so it's less a specific number and more about how much harm you're doing yourself, but I don't know if that's possible...

Agreed. Can you point me to that graphing site?

yeah, sorry, we put the parameters for the curve at the top of each of the audits that use them, e.g.

lighthouse/lighthouse-core/audits/first-meaningful-paint.js

Lines 24 to 27 in 3043cc3

// Parameters (in ms) for log-normal CDF scoring. To see the curve:

// https://www.desmos.com/calculator/joz3pqttdq

const SCORING_POINT_OF_DIMINISHING_RETURNS = 1600;

const SCORING_MEDIAN = 4000;

you can edit the graph and then save it, which will give a new permalink for that particular graph to add as a comment. You have to create an account to save, but it can login via github :)

brendankenny · 2017-02-10T22:26:31Z

lighthouse-core/audits/dobetterweb/num-dom-nodes.js

+      description: 'Uses a small number of DOM nodes',
+      helpText: 'Browser engineers recommend pages contain fewer than ' +
+          `~${NumDOMNodes.MAX_DOM_NODES} DOM nodes. The sweet spot is around 60 elements wide x ` +
+          '32 elements deep. A large DOM can increase memory, cause longer ' +


should we be looking at length of the longest paths, or distribution of path lengths in the DOM?

brendankenny · 2017-02-10T22:30:32Z

lighthouse-core/gather/gatherers/dobetterweb/domnodes.js

+   * @return {!Promise<!Array<!Object>>}
+   */
+  afterPass(options) {
+    return options.driver.querySelectorAll('body, body /deep/ *');


agree with @patrickhulce that this may be a really big thing to bring back over and keep around just for the length. It doesn't look like the array contents would be particularly helpful for others audits, just an array of node IDs that would then each be wrapped by Elements, but we wouldn't be able to tell them apart in any way without more protocol requests for each one...

Particularly if you're going to have to get tricky with a selector for Shadow DOM v1, maybe switch to evaluateAsync and ship back only the number of nodes?

brendankenny · 2017-02-10T22:33:46Z

lighthouse-core/gather/gatherers/dobetterweb/domnodes.js

+   * @return {!Promise<!Array<!Object>>}
+   */
+  afterPass(options) {
+    return options.driver.querySelectorAll('body, body /deep/ *');


We could get trickier and create a shared driver method that returns a promise and caches the result after the first call. Thoughts?

We probably should measure, I guess :) Maybe it's not so bad.

Beyond perf, though, for this gatherer there doesn't seem to be any benefit to keeping the array around

ebidel · 2017-02-14T01:09:04Z

Giving this a big time beef up. Stay tuned.

ebidel · 2017-02-16T02:19:04Z

PTAL. The audit UI has changed quite a bit, including a new formatter, and now being a scored audit. Take a look at the description.

patrickhulce

this is really cool, I dig the cards! just a few minor comments

patrickhulce · 2017-02-16T16:54:45Z

lighthouse-core/formatters/partials/cards.html

@@ -0,0 +1,8 @@
+<ul class="subitem__details cards__container">
+  {{#each this}}
+    <div class="subitem__detail scorecard" {{#if snippet}}title="{{snippet}}"{{/if}}>


if it will always be the title can we rename snippet to title?

I reserved title for the card title. Also wanted to make it obvious this was a code snippet.

oh whoops, alright

patrickhulce · 2017-02-16T16:56:49Z

lighthouse-core/gather/gatherers/dobetterweb/domstats.js

+  }
+  const className = element.className;
+  if (className) {
+    name += `.${className.split(' ').join('.')}`;


maybe just use replace here?

patrickhulce · 2017-02-16T17:01:40Z

lighthouse-core/gather/gatherers/dobetterweb/domstats.js

+      path.push(createSelectorsLabel(node));
+    }
+  }
+  return path.reverse();


I don't particularly mind which one, but any gotchas with unshift instead?

that works. done.

patrickhulce · 2017-02-16T17:04:37Z

lighthouse-core/gather/gatherers/dobetterweb/domstats.js

+   * @return {!Promise<!Array<!Object>>}
+   */
+  afterPass(options) {
+    return options.driver.querySelectorAll('body, body /deep/ *')


do we still need to do this if we're already calling evaluateAsync? could probably just return in getDOMStats without shipping all the nodes.

good call. done.

patrickhulce · 2017-02-16T17:06:08Z

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js

@@ -149,6 +149,36 @@ module.exports = [
      }
    }
  }, {
+    initialUrl: 'http://localhost:10200/dobetterweb/domtester.html?smallDOM',


yay for moving out into some new testers! 🎉

paulirish · 2017-02-16T18:55:37Z

lighthouse-core/audits/dobetterweb/dom-size.js

+    const cards = [
+      {title: 'Total DOM Nodes', value: stats.totalDOMNodes.toLocaleString()},
+      {title: 'Max DOM Depth', value: stats.depth.max.toLocaleString(), snippet: depthSnippet},
+      {title: 'Max Children / Node', value: stats.width.max.toLocaleString(), snippet: widthSnippet}


"Max Children / Node" doesn't read too well to me. Seems more like a rate of speed or something.

how about "Maximum Child Nodes"

"Maximum Children". Done

paulirish · 2017-02-16T18:56:00Z

lighthouse-core/audits/dobetterweb/dom-size.js

+
+    const cards = [
+      {title: 'Total DOM Nodes', value: stats.totalDOMNodes.toLocaleString()},
+      {title: 'Max DOM Depth', value: stats.depth.max.toLocaleString(), snippet: depthSnippet},


while this is the maximum depth i guess its also just the total DOM depth. how about "DOM Depth"

paulirish · 2017-02-16T18:58:02Z

lighthouse-core/gather/gatherers/dobetterweb/domstats.js

+ * @return {!string}
+ */
+/* istanbul ignore next */
+function createSelectorsLabel(element) {


can we use this to show who is the parent of the most childNodes in the scorecard?

It does. Here's what it looks like:

ebidel · 2017-02-17T02:31:48Z

PTAL

ebidel added DoBetterWeb new_audit labels Feb 9, 2017

patrickhulce reviewed Feb 9, 2017

View reviewed changes

ebidel force-pushed the domnodes branch from 5b3829f to 4a2f226 Compare February 9, 2017 18:36

brendankenny requested changes Feb 10, 2017

View reviewed changes

ebidel added 9 commits February 15, 2017 16:26

Audit: num DOM nodes created by the page

dbec36e

update gatherer

52f7034

More useful audit

dc798e5

Add card formatter

c4b4370

Cleanup

3c367b8

audit file rename

1ec26e0

Update audit tests. Add pretty print support for card formatter

c037cb8

css tweaks

6531bd5

Smoketests, card formatter tests

08346cf

ebidel force-pushed the domnodes branch from 4a2f226 to 08346cf Compare February 16, 2017 02:14

ebidel changed the title ~~Audit: num DOM nodes created by the page~~ Audit: DOM stats (total nodes, depth, width) Feb 16, 2017

Ignore in page functions in istanbul

22dc064

patrickhulce suggested changes Feb 16, 2017

View reviewed changes

feedback

1d51d6e

patrickhulce approved these changes Feb 16, 2017

View reviewed changes

paulirish requested changes Feb 16, 2017

View reviewed changes

feedback

1eef7bb

paulirish approved these changes Feb 17, 2017

View reviewed changes

ebidel merged commit 220b9f0 into master Feb 17, 2017

ebidel deleted the domnodes branch February 17, 2017 05:14

	// Parameters (in ms) for log-normal CDF scoring. To see the curve:
	// https://www.desmos.com/calculator/joz3pqttdq
	const SCORING_POINT_OF_DIMINISHING_RETURNS = 1600;
	const SCORING_MEDIAN = 4000;

Audit: DOM stats (total nodes, depth, width) #1673

Audit: DOM stats (total nodes, depth, width) #1673

Conversation

ebidel commented Feb 9, 2017 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Feb 14, 2017 • edited Loading

ebidel commented Feb 16, 2017

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Feb 17, 2017

ebidel commented Feb 9, 2017 •

edited

Loading

ebidel Feb 10, 2017 •

edited

Loading

ebidel commented Feb 14, 2017 •

edited

Loading