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

perf: improve blocked and masking logic checks #1536

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/afraid-knives-relate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"rrweb": patch
---

improve blocking and masking perf
16 changes: 8 additions & 8 deletions packages/rrdom/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -351,11 +351,8 @@

getId(n: RRNode | undefined | null): number {
if (!n) return -1;

const id = this.getMeta(n)?.id;

// if n is not a serialized Node, use -1 as its id.
return id ?? -1;
return this.getMeta(n)?.id ?? -1;
JonasBa marked this conversation as resolved.
Show resolved Hide resolved
}

getNode(id: number): RRNode | null {
Expand All @@ -373,11 +370,14 @@
// removes the node from idNodeMap
// doesn't remove the node from nodeMetaMap
removeNodeFromMap(n: RRNode) {
const id = this.getId(n);
this.idNodeMap.delete(id);
const queue = [n];
while (queue.length > 0) {
const n = queue.pop()!;

Check warning on line 375 in packages/rrdom/src/index.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrdom/src/index.ts#L375

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
this.idNodeMap.delete(this.getId(n));

if (n.childNodes) {
n.childNodes.forEach((childNode) => this.removeNodeFromMap(childNode));
for (let i = 0; i < n.childNodes.length; i++) {
queue.push(n.childNodes[i]);
}
}
}
has(id: number): boolean {
Expand Down
28 changes: 9 additions & 19 deletions packages/rrweb-snapshot/src/snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,27 +280,17 @@
// should warn? maybe a text node isn't attached to a parent node yet?
return false;
} else {
el = dom.parentElement(node)!;

Check warning on line 283 in packages/rrweb-snapshot/src/snapshot.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/snapshot.ts#L283

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
}
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

try/catch is an optimization bailout reason in v8. It's unclear exactly why this code is here, but since this is an extremely hot path, we should remove it

if (typeof maskTextClass === 'string') {
if (checkAncestors) {
if (el.closest(`.${maskTextClass}`)) return true;
} else {
if (el.classList.contains(maskTextClass)) return true;
}
} else {
if (classMatchesRegex(el, maskTextClass, checkAncestors)) return true;
}
if (maskTextSelector) {
if (checkAncestors) {
if (el.closest(maskTextSelector)) return true;
JonasBa marked this conversation as resolved.
Show resolved Hide resolved
} else {
if (el.matches(maskTextSelector)) return true;
}
}
} catch (e) {
//
if (typeof maskTextClass === 'string') {
if (el.matches(`.${maskTextClass}`)) return true;
if (checkAncestors && el.matches(`.${maskTextClass} *`)) return true;
} else {
if (classMatchesRegex(el, maskTextClass, checkAncestors)) return true;
}
if (maskTextSelector) {
if (el.matches(maskTextSelector)) return true;
if (checkAncestors && el.matches(`${maskTextSelector} *`)) return true;
}
return false;
}
Expand Down Expand Up @@ -700,10 +690,10 @@
const recordInlineImage = () => {
image.removeEventListener('load', recordInlineImage);
try {
canvasService!.width = image.naturalWidth;

Check warning on line 693 in packages/rrweb-snapshot/src/snapshot.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/snapshot.ts#L693

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
canvasService!.height = image.naturalHeight;

Check warning on line 694 in packages/rrweb-snapshot/src/snapshot.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/snapshot.ts#L694

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
canvasCtx!.drawImage(image, 0, 0);

Check warning on line 695 in packages/rrweb-snapshot/src/snapshot.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/snapshot.ts#L695

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
attributes.rr_dataURL = canvasService!.toDataURL(

Check warning on line 696 in packages/rrweb-snapshot/src/snapshot.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb-snapshot/src/snapshot.ts#L696

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
dataURLOptions.type,
dataURLOptions.quality,
);
Expand Down
10 changes: 5 additions & 5 deletions packages/rrweb/src/record/mutation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -362,13 +362,13 @@
};

while (this.mapRemoves.length) {
this.mirror.removeNodeFromMap(this.mapRemoves.shift()!);
this.mirror.removeNodeFromMap(this.mapRemoves.pop()!);

Check warning on line 365 in packages/rrweb/src/record/mutation.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/record/mutation.ts#L365

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
}

for (const n of this.movedSet) {
if (
isParentRemoved(this.removes, n, this.mirror) &&
!this.movedSet.has(dom.parentNode(n)!)

Check warning on line 371 in packages/rrweb/src/record/mutation.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/record/mutation.ts#L371

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
) {
continue;
}
Expand Down Expand Up @@ -584,8 +584,8 @@
});
}
if (
isBlocked(m.target, this.blockClass, this.blockSelector, false) ||
value === m.oldValue
value === m.oldValue ||
isBlocked(m.target, this.blockClass, this.blockSelector, false)
) {
return;
}
Expand Down Expand Up @@ -701,9 +701,9 @@
? this.mirror.getId(dom.host(m.target))
: this.mirror.getId(m.target);
if (
isBlocked(m.target, this.blockClass, this.blockSelector, false) ||
isIgnored(n, this.mirror, this.slimDOMOptions) ||
!isSerialized(n, this.mirror)
!isSerialized(n, this.mirror) ||
isBlocked(m.target, this.blockClass, this.blockSelector, false)
) {
return;
}
Expand Down
5 changes: 2 additions & 3 deletions packages/rrweb/src/record/processed-node-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ export default class ProcessedNodeManager {

public inOtherBuffer(node: Node, thisBuffer: MutationBuffer) {
const buffers = this.nodeMap.get(node);
return (
buffers && Array.from(buffers).some((buffer) => buffer !== thisBuffer)
);
if (!buffers) return false;
return !buffers.has(thisBuffer);
}

public add(node: Node, buffer: MutationBuffer) {
Expand Down
29 changes: 11 additions & 18 deletions packages/rrweb/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
'now you can use replayer.getMirror() to access the mirror instance of a replayer,' +
'\r\n' +
'or you can use record.mirror to access the mirror instance during recording.';
/** @deprecated */

Check warning on line 34 in packages/rrweb/src/utils.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/utils.ts#L34

[tsdoc/syntax] tsdoc-missing-deprecation-message: The @deprecated block must include a deprecation message, e.g. describing the recommended alternative
export let _mirror: DeprecatedMirror = {
map: {},
getId() {
Expand Down Expand Up @@ -115,7 +115,7 @@
set(value) {
// put hooked setter into event loop to avoid of set latency
setTimeout(() => {
d.set!.call(this, value);

Check warning on line 118 in packages/rrweb/src/utils.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/utils.ts#L118

[@typescript-eslint/no-non-null-assertion] Forbidden non-null assertion.
}, 0);
if (original && original.set) {
original.set.call(this, value);
Expand All @@ -128,7 +128,7 @@

// copy from https://github.com/getsentry/sentry-javascript/blob/b2109071975af8bf0316d3b5b38f519bdaf5dc15/packages/utils/src/object.ts
export function patch(
source: { [key: string]: any },

Check warning on line 131 in packages/rrweb/src/utils.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/rrweb/src/utils.ts#L131

[@typescript-eslint/no-explicit-any] Unexpected any. Specify a different type.
name: string,
replacement: (...args: unknown[]) => unknown,
): () => void {
Expand Down Expand Up @@ -247,29 +247,22 @@
blockSelector: string | null,
checkAncestors: boolean,
): boolean {
if (!node) {
return false;
}
const el = closestElementOfNode(node);
if (!el) return false;
if (!blockClass && !blockSelector) return false;

if (!el) {
return false;
}

try {
if (typeof blockClass === 'string') {
if (el.classList.contains(blockClass)) return true;
if (checkAncestors && el.closest('.' + blockClass) !== null) return true;
} else {
if (classMatchesRegex(el, blockClass, checkAncestors)) return true;
}
} catch (e) {
// e
}
if (blockSelector) {
if (el.matches(blockSelector)) return true;
if (checkAncestors && el.closest(blockSelector) !== null) return true;
if (checkAncestors && el.matches(`${blockSelector} *`)) return true;
}

if (typeof blockClass === 'string') {
if (el.matches(`.${blockClass}`)) return true;
if (checkAncestors && el.matches(`.${blockClass} *`)) return true;
} else {
if (classMatchesRegex(el, blockClass, checkAncestors)) return true;
}

return false;
}

Expand Down
Loading