feat: add --fast mode #49

patrickhulce · 2017-05-08T23:40:44Z

This PR changes the progress calculation to optionally skip frames that are between ones that have similar progress toward the target. When enabled, it speeds up processing by about 40% and has minimal impact when not enabled via the shortcut. This also aligns perceptual progress and visual progress by sharing the same computation code and fixes #48 by virtue of the fact that we can't wait for the global min when we only parse some of the JPEGs.

Timings

( for i in $(seq 10); do; speedline cnn.json --pretty; done; )  55.15s user 4.63s system 98% cpu 1:00.44 total

( for i in $(seq 10); do; speedline cnn.json --pretty --fast; done; )  34.28s user 3.32s system 99% cpu 37.737 total

1 - (34/55) = ~38%

Open Questions

Expose the direct threshold value as the option or keep as binary? If so, name? similarProgressThreshold?

pahammad · 2017-05-08T23:48:12Z

Skipping frames can artificially make the visual jitter (jankiness) go away from the calculations. Please make sure that layout instability effects that perceptual speed index captures aren't sacrificed due to this frame-skipping (technically, this is temporal downsampling).

patrickhulce · 2017-05-08T23:54:35Z

@pahammad by its nature, the impact on the indexes cannot be avoided as you point out. However, in my experience there are a number of issues with using these metrics too as a signal of layout stability in the first place and frequently I've seen jitter artificially be rewarded by speed index rather than punished. Would a warning and tunable threshold address your concerns or are you against including the option at all with these drawbacks?

pahammad · 2017-05-09T00:03:43Z

I am OK with including the frame-skipping as an option if it comes with appropriate warning when the option is exercised. I am very curious about your sentence: "frequently I've seen jitter artificially be rewarded by speed index rather than punished". Are you referring to the classical (histogram-based) speed index, or are you referring to SSIM based perceptual speed index (PSI) ? If you are referring to PSI in that sentence (where it rewards jitter instead of punishing it), I would love to see an example or two - this is opposite to what I've seen.

patrickhulce · 2017-05-09T17:11:51Z

@pahammad disclaimer text added and I've filed #50 to discuss layout stability tracking 👍

paulirish

thanks for doing this. will be a big help.

paulirish · 2017-05-09T17:00:18Z

test/speed-index.js

@@ -136,6 +136,15 @@ test('speed indexes calculated for realistic trace', async t => {
 	t.is(Math.floor(indexes.perceptualSpeedIndex), 578);
 });

+test('speed indexes calculated for realistic trace with --fast', async t => {


let's also assert that we didn't call getPerceptualProgress, etc on every frame, since that's the essence of this.

paulirish · 2017-05-09T17:01:57Z

src/speed-index.js

+		frames,
+		0,
+		frames.length - 1,
+		opts && opts.fast ? 5 : 0,


set 5 as a constant above?

reading this line it's hard to understand what "5" as a "threshold" even means.

yup will do

paulirish · 2017-05-09T17:04:51Z

src/speed-index.js

+		0,
+		frames.length - 1,
+		opts && opts.fast ? 5 : 0,
+		frame => {


extract this and the next fn into named variables

paulirish · 2017-05-09T17:12:10Z

src/speed-index.js

+		0,
+		frames.length - 1,
+		opts && opts.fast ? 5 : 0,
+		frame => frame.getProgress() || calculateFrameProgress(frame, initial, target),


maybe not enough coffee, but ... brain hurts.

this is an expression of fn1 || fn2 and will be passed in as the getProgress parameter.
when called, the fn1 will evaluate and then return and if that result is falsy then the fn2 will then run?

is there another way to model this?

paulirish · 2017-05-09T17:20:58Z

src/speed-index.js

+
+	if (Math.abs(lowerProgress - upperProgress) < threshold) {
+		for (let i = lowerBound; i < upperBound; i++) {
+			setProgress(frames[i], lowerProgress);


should we also set a property on the frame indicating this value was interpolated? just an idea.

yeah good plan, make the assertion tests easy too

paulirish · 2017-05-09T17:23:46Z

src/speed-index.js

+	const lowerProgress = getProgress(lowerFrame);
+	const upperProgress = getProgress(upperFrame);
+
+	setProgress(lowerFrame, lowerProgress);


should we just put the setProgress call inside of our custom getProgress fn?

that seems to breakdown the perf win of differentiating between setting the progress and parsing the jpeg to get progress, no?

well i meant we cache the result value and only do the work if necessary.
but admittedly there isn't a nice way to do this with the frame object API.

aiight nevermind.

paulirish · 2017-05-09T17:28:04Z

src/speed-index.js

+	setProgress(lowerFrame, lowerProgress);
+	setProgress(upperFrame, upperProgress);
+
+	if (Math.abs(lowerProgress - upperProgress) < threshold) {


as discussed earlier i'm thinking we might want to threshold on time as well.

const FastMode_FrameProgressDelta = 5; // units: percent const FastMode_TimeDelta = 200; // units: ms

wdyt?
this way we can handle a 5% change over 5 seconds (probably worth inspecting) vs a 5% change over 100ms (not worth it)

yeah I like it, but it'll likely erase a lot of the perf win if it's just straight up &&. In the CNN case, the same exact image is used across ~10s of the trace which accounts for ~30 frames. Thoughts on combining progress and time?

wdyt of using that fun exponentiation window size function from TTFI?

allowable percent change = f(time elapsed in seconds) = 4 * e^(-.69 * t) + 1 f(0) = 4 + 1 = 5% allowed change when elapsed time is 0s f(1) = 2 + 1 = 3% allowed change when elapsed time is 1s f(∞) = 0 + 1 = 1% allowed change when elapsed time is very large

yeah i like that approach.

paulirish · 2017-05-10T04:20:55Z

src/speed-index.js

@@ -2,6 +2,31 @@

 const imageSSIM = require('image-ssim');

+/* BEGIN FAST MODE CONSTANTS - See function doc for explanation */
+const FAST_MODE_ALLOWABLE_CHANGE_MAX = 5;


speedline doesnt neccessarily follow the google js style guide.

i'd love to take that opportunity to not caps_lock our constants :)

You good with something like... fastModeAllowableChangeMax ?

paulirish · 2017-05-10T04:22:34Z

src/speed-index.js

+ *
+ *  f(t) = FAST_MODE_MULTIPLIER * e^(FAST_MODE_EXPONENTIATION_COEFFICIENT * t) + FAST_MODE_CONSTANT
+ */
+function calculateFastModeAllowableChange(elapsedTime) {


can we add a few tests for calculateFastModeAllowableChange to just to confirm our landmark values give the right results

paulirish · 2017-05-10T04:25:48Z

cli.js

+	console.warn('WARNING: using --fast may result in different metrics due to skipped frames');
+}
+
+speedIndex(filePath, {fast: cli.flags.fast}).then(function (res) {


let's change this opt property name from fast to fastMode

paulirish · 2017-05-10T04:26:29Z

test/speed-index.js

+	t.is(Math.floor(indexes.speedIndex), 4360);
+	t.is(Math.floor(indexes.perceptualSpeedIndex), 4360);
+
+	// Ensure we skipped computation of most of the 60 fps frames


loooooooove it

paulirish · 2017-05-10T06:53:54Z

changes lgtm but 2 test failures we need to work out.

patrickhulce · 2017-05-10T16:18:12Z

changes lgtm but 2 test failures we need to work out.

done

paulirish · 2017-05-11T01:03:23Z

[email protected] published with this.

perf: add --fast mode

9b37e78

paulirish mentioned this pull request May 8, 2017

[meta] I/O The Final Countdown GoogleChrome/lighthouse#2161

Closed

40 tasks

added disclaimer

4f45fff

patrickhulce mentioned this pull request May 9, 2017

Indexes view layout instability as positive progress #50

Open

paulirish requested changes May 9, 2017

View reviewed changes

patrickhulce added 2 commits May 9, 2017 16:22

feedback and better tests

3ba0355

add comments

0181d6f

paulirish reviewed May 10, 2017

View reviewed changes

feedback

0a42f63

paulirish approved these changes May 10, 2017

View reviewed changes

lint

7e625df

paulirish approved these changes May 11, 2017

View reviewed changes

paulirish merged commit 4998ef6 into paulirish:master May 11, 2017

paulirish mentioned this pull request May 11, 2017

bump speedline version and enable fastMode GoogleChrome/lighthouse#2227

Closed

patrickhulce deleted the fast_mode branch August 29, 2017 23:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add --fast mode #49

feat: add --fast mode #49

patrickhulce commented May 8, 2017 •

edited

Loading

pahammad commented May 8, 2017

patrickhulce commented May 8, 2017 •

edited

Loading

pahammad commented May 9, 2017

patrickhulce commented May 9, 2017

paulirish left a comment

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017

paulirish May 9, 2017

paulirish May 9, 2017

patrickhulce May 9, 2017 •

edited

Loading

paulirish May 9, 2017

paulirish May 10, 2017

patrickhulce May 10, 2017

paulirish May 10, 2017

patrickhulce May 10, 2017

paulirish May 10, 2017

patrickhulce May 10, 2017

paulirish May 10, 2017

paulirish commented May 10, 2017

patrickhulce commented May 10, 2017

paulirish commented May 11, 2017

feat: add --fast mode #49

feat: add --fast mode #49

Conversation

patrickhulce commented May 8, 2017 • edited Loading

pahammad commented May 8, 2017

patrickhulce commented May 8, 2017 • edited Loading

pahammad commented May 9, 2017

patrickhulce commented May 9, 2017

paulirish 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

patrickhulce May 9, 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

paulirish commented May 10, 2017

patrickhulce commented May 10, 2017

paulirish commented May 11, 2017

patrickhulce commented May 8, 2017 •

edited

Loading

patrickhulce commented May 8, 2017 •

edited

Loading

patrickhulce May 9, 2017 •

edited

Loading