Skip to content

Screenshotter --fast argument speeds it up 6x.#752

Merged
k4b7 merged 4 commits intoKaTeX:masterfrom
kohler:faster-screenshotter
Jul 4, 2017
Merged

Screenshotter --fast argument speeds it up 6x.#752
k4b7 merged 4 commits intoKaTeX:masterfrom
kohler:faster-screenshotter

Conversation

@kohler
Copy link
Copy Markdown
Collaborator

@kohler kohler commented Jun 30, 2017

The slowest part of screenshotter tests is the page load, probably
because so many assets must be loaded over the slow docker
connection. On my laptop this takes ~4s per test. The --fast
argument avoids this cost by rendering new TeX on the existing
page. For second and subsequent tests, use executeAsyncScript
to call KaTeX, rather than performing a full page + asset load.
(If too many errors happen in --verify mode, we fall back to
full loads.)

On my laptop, a full verify (chrome + FF) used to take 12m20s+.
With --fast it takes 2m23s.

The slowest part of screenshotter tests is the page load, probably
because so many assets must be loaded over the slow docker
connection. On my laptop this takes ~4s per test. The `--fast`
argument avoids this cost by rendering new TeX on the existing
page. For second and subsequent tests, use `executeAsyncScript`
to call KaTeX, rather than performing a full page + asset load.
(If too many errors happen in `--verify` mode, we fall back to
full loads.)

On my laptop, a full verify (chrome + FF) used to take 12m20s+.
With `--fast` it takes 2m23s.
@kohler
Copy link
Copy Markdown
Collaborator Author

kohler commented Jun 30, 2017

This does not yet update travis to supply --fast. Most likely --fast should be the default but that could come later I dunno.

@kohler
Copy link
Copy Markdown
Collaborator Author

kohler commented Jun 30, 2017

Works on Travis too so I made --fast the default (old behavior recoverable with --slow).

@kohler kohler mentioned this pull request Jun 30, 2017
@edemaine
Copy link
Copy Markdown
Member

Tested locally and looks good to me! (3m8s) I tried tweaking ss_data.yaml, introducing errors, etc. and all seems to behave as before.

@gagern, do you want to take a look at this to confirm?

@k4b7
Copy link
Copy Markdown
Member

k4b7 commented Jun 30, 2017

@kohler I'm super excited about this change. A lot of time and effort has gone into our current solution so I'm glad that you were able to iterate on it to speed it up in a reliable way.

@gagern
Copy link
Copy Markdown
Collaborator

gagern commented Jul 1, 2017

Had a look at the code, didn't run it. Approach looks sane enough, particularly with the fallback in place. Name of the option could be more descriptive. Perhaps --reload instead of --slow?

@kohler
Copy link
Copy Markdown
Collaborator Author

kohler commented Jul 1, 2017

Sure, good choice—renamed.

@k4b7 k4b7 self-requested a review July 4, 2017 01:19
Copy link
Copy Markdown
Member

@k4b7 k4b7 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

document.getElementById("post").innerHTML = query["post"] || "";

if (callback && document.fonts && document.fonts.ready) {
document.fonts.ready.then(callback);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't realize there was a document.fonts.ready was a thing. I've used webfontloader in the past, but this is better for this use.

function loadMath() {
if (!opts.reload && driverReady) {
driver.executeAsyncScript(
"var callback = arguments[arguments.length - 1]; " +
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had to look this up in the selenium docs. It seems like such an awkward API for interacting with the browser. Thanks for figuring out all of this stuff.

@k4b7 k4b7 merged commit 1704d3b into KaTeX:master Jul 4, 2017
@kohler kohler deleted the faster-screenshotter branch July 5, 2017 15:51
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.

4 participants