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

core(gather-runner): load a blank data URI, rather than about:blank #4310

Merged
merged 13 commits into from
Feb 6, 2018

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Jan 19, 2018

fixes #3707

This is a lot nicer. This page definitely fires onload and DCL, and does so fast. (~35ms after navigation on my machine)

In total this saves 500ms on this run: time lighthouse -G http://example.com

This also deprecates config.blankDuration, which WPT is using..
Also fwiw their blankPage is this: <html><head><style>body {background-color: #DE640D;}</style></head></html>

@patrickhulce how you want to handle the private method? shall we expose with the same API?

// The real about:blank doesn't fire onload and is full of mysteries
// https://github.com/whatwg/html/issues/816#issuecomment-288931753
// To improve speed and avoid anomalies, we use a basic data uri blank page
url = url || 'data:text/html,<!doctype html><title>Resetting Page...</title>';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually kind of love that WPT's is a non-white color to signal when Chrome gets the first byte by turning white but not sure if that will cause issues :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

sweeeeeeet, I'm curious if all the strange local failures have disappeared with this now. Last time I was trying to land this I could never get the perf config to have screenshots locally for some unknown reason. So stoked to land this though. I'll play around a bit before the 👍

Re: WPT

I'm fairly certain that that agent is totally dead at this point, and he doesn't use a custom config in the new version at all so we should be good there. It wouldn't be the end of the world to keep it for now though and just change the default config to wait 0ms.

https://github.com/WPO-Foundation/wptagent/blob/d0815424879f44be9586dc3d41af00f7e0eca483/internal/devtools_browser.py#L389-L397

@paulirish
Copy link
Member Author

paulirish commented Jan 19, 2018 via email

@paulirish
Copy link
Member Author

paulirish commented Jan 20, 2018

Alright... new version updated. and no longer is blank, but branded! ;)

Click to view the GIF:

@hwikyounglee @vinamratasingal can you dig it?

@wardpeet
Copy link
Collaborator

The screenshot is amazing! We should have done this earlier

}
</style>
<body>
<img src='data:image/svg+xml;utf8,<svg style="" xmlns="http://www.w3.org/2000/svg" id="Layer_1" data-name="Layer 1" width="750" height="750"><defs id="defs3"><style id="style5">.cls-2{fill:#ffe082;opacity:.5}.cls-3{fill:#2979ff}.cls-5{fill:#fff176}.cls-6{fill:#f4511e}.cls-7{fill:#e64a19}.cls-9{fill:#ff7043}.cls-11{fill:#fdd835}.cls-12{fill:#fff9c4}</style></defs><path d="M92.428 571.024c52.32 0 52.32-33.94 104.64-33.94s52.31 33.94 104.63 33.94c52.32 0 52.32-33.94 104.63-33.94 52.31 0 52.32 33.94 104.64 33.94s52.32-33.94 104.64-33.94c49.48 0 52.17 30.34 96.56 33.64a326.73 326.73 0 0 0 8.09-72.39c0-179.87-145.82-325.69-325.69-325.69s-325.7 145.82-325.7 325.69a326.75 326.75 0 0 0 7.9 71.51 98.88 98.88 0 0 0 15.66 1.18z" id="path9" fill="#304ffe"/><path id="rect15" fill="#ffd54f" d="M362.978 213.564h84.84v78.49h-84.84z"/><path class="cls-5" id="rect17" fill="#fff176" d="M362.978 213.564h29.95v78.49h-29.95z"/><ellipse class="cls-5" cx="392.298" cy="233.214" rx="19.84" ry="24.89" id="ellipse19" fill="#fff176"/><path class="cls-6" d="M360.898 204.704a43.835 43.835 0 1 1 87.67 0" id="path21" fill="#f4511e"/><path class="cls-7" d="M405.108 160.874a43.51 43.51 0 0 1 43.46 43.83h-43.46v-43.83z" id="path23" fill="#e64a19"/><rect class="cls-6" x="352.418" y="203.294" width="104.64" height="11.31" rx="5.66" ry="5.66" id="rect25" fill="#f4511e"/><path id="polygon27" transform="translate(-42.202 118.024)" fill="#c5cae9" d="M393 416l12.23-242.5h84.84L501 404z"/><path class="cls-9" id="line29" fill="#ff7043" d="M449.038 310.254l3.26 64.17"/><path class="cls-9" id="polygon31" transform="translate(-42.202 118.024)" fill="#ff7043" d="M402.05 233.8l-3.42 67.29 95.87-44.69-3.26-64.17z"/><path class="cls-9" id="line33" fill="#ff7043" d="M455.518 437.664l3.27 64.18"/><path class="cls-9" id="polygon35" transform="translate(-42.202 118.024)" fill="#ff7043" d="M395.25 367.4l-3.42 67.3 109.16-50.88-3.27-64.18z"/><path class="cls-7" id="rect39" fill="#e64a19" d="M349.988 255.904h109.49v35.82h-109.49z"/><path class="cls-6" id="rect41" fill="#f4511e" d="M349.988 255.904h71.78v35.82h-71.78z"/><path class="cls-2" d="M403.978 255.904c0 13-12.1 23.5-27 23.5s-27-10.52-27-23.5" id="path43" opacity=".5" fill="#ffe082"/><g id="layer1" transform="translate(-111.07 296.27)"><path class="cls-3" d="M653.728-50.456a19.59 19.59 0 0 1 8.32 1.84 34.49 34.49 0 0 1 66.7-9h.12a23.25 23.25 0 0 1 0 46.5h-75.14a19.67 19.67 0 1 1 0-39.34z" id="path71" fill="#2979ff"/><path d="M430.728 137.024a16.6 16.6 0 0 1 7 1.56 29.23 29.23 0 0 1 56.53-7.63h.1a19.71 19.71 0 1 1 0 39.41h-63.63a16.67 16.67 0 1 1 0-33.34z" id="path77" fill="#448aff"/></g><g id="layer2" transform="translate(-111.07 296.27)"><circle class="cls-11" cx="593.868" cy="-88.776" r="3.53" id="circle45" fill="#fdd835"/><circle class="cls-12" cx="624.868" cy="109.624" r="6.13" id="circle47" fill="#fff9c4"/><circle class="cls-12" cx="253.468" cy="53.594" r="6.13" id="circle49" fill="#fff9c4"/><circle class="cls-12" cx="353.418" cy="160.214" r="6.13" id="circle51" fill="#fff9c4"/><circle class="cls-12" cx="598.478" cy="11.644" r="6.13" id="circle53" fill="#fff9c4"/><circle class="cls-12" cx="727.628" cy="169.544" r="6.13" id="circle55" fill="#fff9c4"/><circle class="cls-11" cx="240.268" cy="192.404" r="3.53" id="circle57" fill="#fdd835"/><circle class="cls-11" cx="272.828" cy="121.094" r="3.53" id="circle59" fill="#fdd835"/><circle class="cls-11" cx="294.738" cy="102.714" r="3.53" id="circle61" fill="#fdd835"/><circle class="cls-11" cx="387.348" cy="20.004" r="3.53" id="circle63" fill="#fdd835"/><circle class="cls-11" cx="679.868" cy="30.224" r="3.53" id="circle65" fill="#fdd835"/><circle class="cls-11" cx="818.598" cy="177.654" r="3.53" id="circle67" fill="#fdd835"/><circle class="cls-11" cx="328.678" cy="9.394" r="3.53" id="circle69" fill="#fdd835"/><circle class="cls-11" cx="640.898" cy="179.204" r="3.53" id="circle73" fill="#fdd835"/><circle class="cls-11" cx="747.868" cy="90.754" r="3.53" id="circle75" fill="#fdd835"/></g><g id="layer3" transform="translate(-111.07 296.27)"><path class="cls-2" d="M475.488-83.896l-334.62-47.79a3.65 3.65 0 0 0-1-.14c-8.67 0-16 31.89-16 71.15 0 39.26 7.33 71.07 16 71.07a3.66 3.66 0 0 0 .93-.13l334.64-47.88v-46.28z" id="path11" opacity=".5" fill="#ffe082"/></g><g id="layer6"><path id="polygon37" transform="translate(-42.202 118.024)" opacity=".5" fill="#304ffe" d="M493.27 173.5h-42.9V393L502 416l-8.73-242.5z"/></g><g id="layer5" transform="translate(-111.07 296.27)"><path d="M413.768 275.484c52.32 0 52.32-33.94 104.63-33.94 52.31 0 52.32 33.94 104.63 33.94 44.42 0 51.13-24.46 84.16-31.84-45.13-24.66-112.53-40.33-187.84-40.33-75.63 0-143.28 15.79-188.41 40.64 31.93 7.73 39.02 31.53 82.83 31.53z" id="path79" fill="#00c853"/><path d="M413.868 275.014c52.32 0 52.32-33.94 104.63-33.94h1.1l-.58-37.32c-74.89 0-142 15.49-187.08 39.91 31.16 7.98 38.55 31.35 81.93 31.35z" id="path81" fill="#64dd17"/></g></svg>'>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we can have an svg instead of img here: https://gist.github.com/kdzwinel/3912e659a13629bf0de7b0611953affe

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@vinamratasingal-zz
Copy link

vinamratasingal-zz commented Jan 23, 2018

Yup this LGTM'es to me.

EDIT: This is awesome, thanks so much Paul and Patrick for pulling this together :)

static loadBlank(driver, url) {
// The real about:blank doesn't fire onload and is full of mysteries
// https://github.com/whatwg/html/issues/816#issuecomment-288931753
// To improve speed and avoid anomalies, we use a basic data uri blank page
Copy link
Collaborator

Choose a reason for hiding this comment

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

can probably reference #1449 (comment) too?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@paulirish
Copy link
Member Author

Updated.

I got to wrestle with a phantom/nonexisting onload event. The result is what you see in the PR now. Two separate data uri pages.

Using about:blank for the first one could have worked, too. But I really didn't like the flash of white.

PTAL

// Only navigating to a single data-uri doesn't reliably trigger onload. (Why? Beats me.)
// Two data uris work, however the two need to be sufficiently different (Why? Beats me.)
// If they are too similar, Chrome considers the latter to be as superficial as a pushState
// Lastly, it's possible for two navigations to be racy, so we await onload inbetween.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe throw a note in here that it's still not that long since onload happens real fast for these suckers.

Also, if we eliminate the double loadBlank call at the beginning we would've been fine too, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, if we eliminate the double loadBlank call at the beginning we would've been fine too, right?

Hmmm interesting. I gotta look into why we added it: #850

Copy link
Collaborator

Choose a reason for hiding this comment

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

well I meant introducing something more like "if this is the first pass, don't bother trying to navigate to blank since we know we're already on it" not necessarily nuking the first one :)

Copy link
Member Author

Choose a reason for hiding this comment

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

is df175bc what you were thinking?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah yes, it is! though I believe in LR they had to switch the order of passes so we can't necessarily rely on defaultPass being first :/ an index approach seems annoying though.

I'm cool to revisit later if you think that's too much trouble since we've already reduced the time considerably.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah sg. i'll do a followup PR to use the index.

Copy link
Member Author

Choose a reason for hiding this comment

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

included passIndex in this PR. 2 line change. :)

@paulirish paulirish added the OYB label Jan 31, 2018
@paulirish
Copy link
Member Author

Before: 4 calls of loadBlank with a slightly higher timeout. ~1270ms per run
After: 3 calls of loadBlank with a shorter wait. ~630ms per run

With those 600ms of savings, I'll apply the OYB label. 😀

@patrickhulce
Copy link
Collaborator

I'm a little surprised it still takes 200ms for onload of a data URI but I guess we're doing 2 now, so really more like ~100ms?

looks like tests might need a bit of massaging still

@wardpeet
Copy link
Collaborator

does the onload changes when the data-uri is smaller or doesn't it really matter?

@paulirish paulirish added the P0 label Feb 6, 2018
@paulirish
Copy link
Member Author

I'm a little surprised it still takes 200ms for onload of a data URI but I guess we're doing 2 now, so really more like ~100ms?

yes

FWIW i did test a 10kb datauri png instead of this 3kb datauri svg and the png is slower. (~320ms vs ~230ms each loadBlank)

and I did trace the entire thing to attempt to understand whats happening during the 200ms, but it looks like mostly browser process work that I don't see much opportunity in.
trace-of-entire-run.json.zip

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

let's do thissssssss 🏋️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[perf] reduce wasted time on about:blank
6 participants