Speed up tests and other perf improvements #381

GioSensation · 2023-09-11T12:08:41Z

Reviewer: @shakyShane
Asana: https://app.asana.com/0/1198964220583541/1203781196578953/f

Description

A few improvements to speed up test execution and performance in general. In summary:

Mock getComputedStyles in jsdom (60% test speed up 🎉).
Memoize isCCForm: we were calling this for every single input field instead of per-form. This sped things up further, not only in the tests, but also in the real world. Most pages wouldn't see much difference because we have bailouts when there are tons of fields, but in test environments like in fix(perf): avoid checking for CC if we don't need to #258 (with bailouts switched off) this yielded a massive 80+% reduction in execution time.
Disconnect main MutationObserver when we reach the failsafe threshold. Even when we stop adding forms we still do some work, especially in pages like Asana where there's no form so all fields would trigger the expensive search for a form container. This change disconnects the observer altogether, so we no longer incur that extra cost after bailing.

Steps to test

All test suites passing, but much faster! 🎉

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> # Conflicts: # src/Scanner.js

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

jest.config.js

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

jest.setup.js

GioSensation · 2023-09-12T06:52:28Z

src/DeviceInterface/ExtensionInterface.js

@@ -57,13 +57,8 @@ class ExtensionInterface extends InterfacePrototype {
 return null
 }

- removeAutofillUIFromPage () {
- super.removeAutofillUIFromPage()
- this.activeForm?.removeAllDecorations()


I centralized removing all decoration and everything else, so we don't need this override anymore. Find it in the scanner changes below.

GioSensation · 2023-09-12T06:55:10Z

src/Form/Form.js

@@ -128,6 +128,9 @@ class Form {
 get isHybrid () {
 return this.formAnalyzer.isHybrid
 }
+ get isCCForm () {
+ return this.formAnalyzer.isCCForm()
+ }


The isCCForm was in the matching class, and was being run on every single field. FormAnalyzer is a more fitting place and allows us to run it once per form, rather than once per field. Massive performance improvement on pages with tons of fields.

src/Form/Form.js

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation · 2023-09-12T07:00:38Z

src/Form/FormAnalyzer.js

+ * @returns {boolean}
+ */
+ isCCForm () {
+ if (this._isCCForm !== undefined) return this._isCCForm


This is the only change to this method, apart from being moved here. Just memoizing the result. When the form needs to be rescanned it follows the same flow as isLogin and isSignup. The analyzer is destroyed and recreated, so form data is kept up to date when needed and only when needed.

src/Form/input-classifiers.test.js

GioSensation · 2023-09-12T07:03:34Z

src/Form/input-classifiers.test.js

 } = testCase

 const testTextString = expectedFailures.length > 0
 ? `should contain ${expectedFailures.length} known failure(s): ${JSON.stringify(expectedFailures)}`
 : `should NOT contain failures`

- it(testTextString, () => {
- const testContent = fs.readFileSync(path.resolve(__dirname, './test-cases', html), 'utf-8')
+ it.concurrent(testTextString, async () => {


This didn't yield any performance gains for some reason. I've even tried chunking the testCases and running them in parallel, but nothing seemed to matter much 🤷‍♂️. Ran out of time for this scope and the gains are already pretty significant.

src/Scanner.js

src/__snapshots__/Scanner.test.js.snap

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

src/autofill-utils.js

GioSensation · 2023-09-12T08:23:47Z

src/autofill-utils.js

 * @returns {boolean}
 */
 function shouldLog () {
+ return readDebugSetting('ddg-autofill-debug')


This is part of the cherry-picking I mentioned above.

src/Scanner.js

shakyShane

No additional comments other than to say thanks, this is amazing work :)

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation · 2023-09-15T14:50:29Z

src/Form/matching.js

- return (string || '')
+ if (!string) return ''
+
+ return (string)
 .replace(/\n/g, ' ')
 .replace(/\s{2,}/g, ' ').trim()


Since we call this a lot of times and pretty often with null, I'm short circuiting early instead of running two replaces and a trim on an empty string every time.

GioSensation · 2023-09-15T14:51:24Z

src/Scanner.js

 this.findEligibleInputs(document)
- window.performance?.mark?.('scanner:init:end')
+ window.performance?.mark?.('initial_scanner:init:end')
+ logPerformance('initial_scanner')


I've created this logPerformance little utility to log performance metrics of every scan, not just the initial one. Works the exact same way.

GioSensation · 2023-09-15T14:54:26Z

src/Scanner.js

+ // Use input.form unless it encloses most of the DOM
+ // In that case we proceed to identify more precise wrappers
+ if (
+ this.forms.has(input.form) || // If we've added the form we've already checked that it's not a page wrapper


By adding this little check we avoid the much more expensive isFormLikelyToBeUsedAsPageWrapper check. This significantly speeds up forms with a lot of input fields. For example, further massive gain on your test page with 1000 fields, down to ~70ms on our browser on my machine (together with the other changes in the last few commits).

GioSensation · 2023-09-15T14:55:28Z

src/Scanner.js

- const previouslyFoundParent = [...this.forms.keys()].find((form) => form.contains(parentForm))
+ if (parentForm instanceof HTMLFormElement && this.forms.has(parentForm)) {
+ // We've met the form, add the input
+ this.forms.get(parentForm)?.addInput(input)


Return early if we've already met the form, so we avoid all the looping below.

GioSensation · 2023-09-15T14:57:33Z

src/Scanner.js

+ // Check if the forms we've seen are either disconnected,
+ // or are parent/child of the currently-found form
+ let previouslyFoundParent, childForm
+ for (const [formEl] of this.forms) {


In production we had 2 destructuring + loops for this stuff. Most importantly, we were not deleting disconnected forms, and since we didn't disconnect the MutationObserver this caused a leak of resources and longer and longer loops over time on certain occasions like on Asana. The loop is now faster, runs only once with no destructuring and we also remove the disconnected forms. Plus of course now we disconnect the observer as well. 💪

GioSensation · 2023-09-15T15:03:17Z

src/autofill-utils.js

+
+ if (el.type === 'image') {
+ return removeExcessWhitespace(el.alt || el.value || el.title || el.name)
+ }


Just trying to shave off everything possible from this. This shows up a lot in the profile, just because it runs so often. Here I've just hoisted that tiny array and consolidated those two instanceof HTMLInputElement checks into one.

GioSensation added 7 commits September 11, 2023 13:53

Mock getComputedStyles to speed up tests

1aef9ba

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Batch form file reading

99b590b

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Memoize isCCForm

3253b7e

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Minimise impact of Array.from

51aa77b

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Limit test logs to surface failures

2338e41

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Centralize scan stopping and disconnect observer

97d4553

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Update snapshots and commit compiled files

87bca48

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

GioSensation self-assigned this Sep 11, 2023

GioSensation added 3 commits September 11, 2023 15:12

Commit compiled code

9edae2f

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

Add performance logging capability

1562715

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> # Conflicts: # src/Scanner.js

Commit files

e66a74e

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>