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

Ema/perf followups #386

Merged
merged 4 commits into from
Oct 2, 2023
Merged

Ema/perf followups #386

merged 4 commits into from
Oct 2, 2023

Conversation

GioSensation
Copy link
Member

@GioSensation GioSensation commented Sep 29, 2023

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

Description

Follow up for the previous PR. This abstracts RegExp.test to a wrapper that checks for string length before executing. This is both to avoid performance spikes and for accuracy (avoiding noise). My new law of software engineering: given a long enough text, your regex will always match.

For more context, on certain specific pages we were running multiple successive regex checks on humongous strings, like 60+k characters. Chrome and Firefox handled that decently, but webkit 17 (macOS Sonoma) spun out of control and basically the whole tab hanged.

Steps to test

Tests added. If you remove the safeguards and add back the selector for title in class and id, the test times out on Webkit 17.

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
@GioSensation GioSensation self-assigned this Sep 29, 2023
@@ -279,30 +279,27 @@ test.describe('macos', () => {
})
test.describe('matching performance', () => {
test('real-world form', async ({page}) => {
await forwardConsoleMessages(page)
Copy link
Member Author

Choose a reason for hiding this comment

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

I've removed the console forwarding from these tests so we can more easily read the performance results of the test itself without digging through lots of noise.

@@ -1,4 +1,4 @@
import {buttonMatchesFormType, getTextShallow} from '../autofill-utils.js'
import {buttonMatchesFormType, getTextShallow, safeRegexTest} from '../autofill-utils.js'
Copy link
Member Author

@GioSensation GioSensation Sep 29, 2023

Choose a reason for hiding this comment

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

I've substituted all RegExp.test instances with a wrapper that checks for string length first. Find it down below. Should probably consider an ESLint rule.

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
@GioSensation GioSensation merged commit 84127e0 into main Oct 2, 2023
1 check passed
@GioSensation GioSensation deleted the ema/perf-followups branch October 2, 2023 09:27
CDRussell pushed a commit to duckduckgo/Android that referenced this pull request Oct 11, 2023
Task/Issue URL:
https://app.asana.com/0/1205691418983700/1205691418983700
Autofill Release:
https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2


## Description
Updates Autofill to version
[8.4.2](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2).

### Autofill 8.4.2 release notes
## What's Changed
* Ema/perf followups by @GioSensation in
duckduckgo/duckduckgo-autofill#386
* Fix handler regression by @GioSensation in
duckduckgo/duckduckgo-autofill#392
* Bump markdown-it from 13.0.1 to 13.0.2 by @dependabot in
duckduckgo/duckduckgo-autofill#388
* Revert mutation observer for forms by @GioSensation in
duckduckgo/duckduckgo-autofill#396


**Full Changelog**:
duckduckgo/duckduckgo-autofill@8.4.1...8.4.2

## Steps to test
This release has been tested during autofill development. For smoke test
steps see [this
task](https://app.asana.com/0/1198964220583541/1200583647142330/f).

Co-authored-by: GioSensation <GioSensation@users.noreply.github.com>
graeme added a commit to duckduckgo/macos-browser that referenced this pull request Oct 11, 2023
Task/Issue URL:
https://app.asana.com/0/1205691418983706/1205691418983706
Autofill Release:
https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2
BSK PR: duckduckgo/BrowserServicesKit#530

## Description
Updates Autofill to version
[8.4.2](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2).

### Autofill 8.4.2 release notes
## What's Changed
* Ema/perf followups by @GioSensation in
duckduckgo/duckduckgo-autofill#386
* Fix handler regression by @GioSensation in
duckduckgo/duckduckgo-autofill#392
* Bump markdown-it from 13.0.1 to 13.0.2 by @dependabot in
duckduckgo/duckduckgo-autofill#388
* Revert mutation observer for forms by @GioSensation in
duckduckgo/duckduckgo-autofill#396


**Full Changelog**:
duckduckgo/duckduckgo-autofill@8.4.1...8.4.2

## Steps to test
This release has been tested during autofill development. For smoke test
steps see [this
task](https://app.asana.com/0/1198964220583541/1200583647142330/f).

---------

Co-authored-by: GioSensation <GioSensation@users.noreply.github.com>
Co-authored-by: Graeme Arthur <2030310+graeme@users.noreply.github.com>
joshliebe pushed a commit to duckduckgo/Android that referenced this pull request Nov 7, 2023
Task/Issue URL:
https://app.asana.com/0/1205691418983700/1205691418983700
Autofill Release:
https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2


## Description
Updates Autofill to version
[8.4.2](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/8.4.2).

### Autofill 8.4.2 release notes
## What's Changed
* Ema/perf followups by @GioSensation in
duckduckgo/duckduckgo-autofill#386
* Fix handler regression by @GioSensation in
duckduckgo/duckduckgo-autofill#392
* Bump markdown-it from 13.0.1 to 13.0.2 by @dependabot in
duckduckgo/duckduckgo-autofill#388
* Revert mutation observer for forms by @GioSensation in
duckduckgo/duckduckgo-autofill#396


**Full Changelog**:
duckduckgo/duckduckgo-autofill@8.4.1...8.4.2

## Steps to test
This release has been tested during autofill development. For smoke test
steps see [this
task](https://app.asana.com/0/1198964220583541/1200583647142330/f).

Co-authored-by: GioSensation <GioSensation@users.noreply.github.com>
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.

2 participants