Improve performance of walk utility

[AbhiPrasad]

We have a walk util which we use to walk objects to normalize them.

sentry-javascript/packages/utils/src/object.ts

Line 312 in 0a6e535

export function walk(key: string, value: any, depth: number = +Infinity, memo: MemoFunc = memoBuilder()): any {

This is used extensively throughout the SDK, primarily through

sentry-javascript/packages/utils/src/object.ts

Lines 373 to 379 in 0a6e535

	export function normalize(input: any, depth?: number): any {
	try {
	return JSON.parse(JSON.stringify(input, (key: string, value: any) => walk(key, value, depth)));
	} catch (_oO) {
	return '**non-serializable**';
	}
	}

@timfish and some others have reported OOM errors on coming from the walk function - which shows that it is not always super safe to use. We've also seen issues like #4470.

We should try to do a couple things here:

1. Profile normalize and walk, see where the bottlenecks are in various scenarios. It's used in electron and react native also, so it'll be interesting to see the effects on that platform.
2. Experiment with adding a limit to the number of leaves, not just limiting serialization depth
3. Explore going beyond using a JSON.Stringify replacer to more a more rigorous object traversal to try to improve efficiency.

We can test potential performance improvements with https://github.com/getsentry/sentry-sdk-benchmark/

In a slightly related note, we also use JSON.stringify a lot throughout the codebase, and it's not always clear when to use walk to normalize. We should make the usage consistent, perhaps by extracting into distinct helpers based on the use case. This also means we could have "more expensive" and "less expensive" normalization functions - perhaps that will just alleviate the issue we have here.

[timfish]

For ref. here is the open Electron issue:
getsentry/sentry-electron#263

I managed to reproduce this once before by inadvertently console logging a huge object which referenced a DOM element. This went through JSON.stringify + walk and caused OOM very quickly even with the walk depth limited to 50.

I've also seen Webpack HMR trigger the bug occasionally with whatever it does on update.

I suspect it is not enough to only limit the depth we serialise since that can still result in huge objects. We should probably be attempting to limit the number of "leaves" too.

[AbhiPrasad]

Here's a related Vue issue: #2957

[productdevbook]

vue 3 some problem.

[AleshaOleg]

Is it still relevant? I can take a look into it, but the walk function isn't there already, so I need some updates on what should be done here

[AbhiPrasad]

Hey @AleshaOleg, thanks for your interest! The function is now called visit, and it's used by normalize which we use through out the SDK:

sentry-javascript/packages/utils/src/normalize.ts

Lines 36 to 43 in 618e992

	export function normalize(input: unknown, depth: number = 100, maxProperties: number = +Infinity): any {
	try {
	// since we're at the outermost level, we don't provide a key
	return visit('', input, depth, maxProperties);
	} catch (err) {
	return { ERROR: `**non-serializable** (${err})` };
	}
	}

Not everything in this issue applies anymore, but we haven't profiled it strictly fwiw, I think it's worth it to take a look at a flamegraph and see the bottlenecks.

Given we've implemented limiting both breadth and depth and we have a more strict object traversal I'm going to go ahead and close this issue. If you do notice anything after running a benchmark let us know, we can re-open/create a new issue and dive back further into this.

[AleshaOleg]

@AbhiPrasad I wasn't able to figure out how to use the newer benchmark repo. Tried remote setup, but seems it's only for Sentry employees, as I need to access https://run.testa.getsentry.net/. And also not sure why there's only Python platform there. If you have any examples of how to set benchmarks for javascript SDK locally please share them, I would appreciate it.

But I tested yesterday with the old benchmark repo - https://github.com/getsentry/sentry-sdk-benchmark/ for the javascript/express platform. I'm not sure which indexes (see screenshot) are good/bad for these three apps that are running there. Anything specific I need to do to check the performance of the normalize function?

But as there's no more JSON.stringify seems like everything working as expected with the 100 limit.

[AbhiPrasad]

Tried remote setup, but seems it's only for Sentry employees

Yeah we haven't opened that up for external contributors yet - cool you took a look though!

I think we need micro-benchmarks instead of the aggregate benchmarks for latency/CPU that our benchmarking repos give us. I would take a look at https://github.com/davidmarkclements/0x, which is a great tool for generating flamegraphs for small node scripts.

[AleshaOleg]

Got it. Because looked through packages/utils/test and didn't find anything specific to benchmarks. And as 0x is not used anywhere in the project, I suppose there are no benchmark tests. Will try to set something there then with 0x

[AleshaOleg]

@AbhiPrasad here are some results I got so far. I created a test (based on this one). You can run it from the root in my branch with this command: 0x -D ./packages/utils/test/0x/output/{pid}.0x node_modules/.bin/ts-node -r ./packages/utils/test/0x/.env ./packages/utils/test/0x/normalize.ts.

Below you can see 5 screenshots. For the first 3 it's running normalize with depth parameter: 100, 1000, and 10000 respectively within 1000 milliseconds (It's 1000 executions. One execution per 1 millisecond. Because of setTimeout here).

As we can see, there's not so much difference if we're changing depth while running only 1000 executions of the normalize function. The stack of the visit execution takes around 3-6%. That can be even an error term, because for a lower number of executions stack ratio is larger and vice versa. Unfortunately, we don't have any other difference in metrics here, except execution time. And stack depth doesn't change even with only 100 executions with 100 depth. However, if we continue to increase the timeframe (and number of executions with it) from 1000 to 10000 and later to 100000 we will see a major difference between them with the same depth of 100. Now visit stacks ratio takes around 23% and 38% respectively from the whole execution. You can see in the screenshots below.

It really depends on how many times in a row normalize function would be called. That's kinda obvious, if we call any function without stopping it will take a longer time to execute each next function. But depth doesn't play a huge role as I understood, at least within the number I tested.

I would say 100 depth is completely safe to use. I would even consider 1000 as safe, but I don't know how and where exactly this function is used and all of the technical details. Correct me if I'm wrong, but as I understood this function is called when a stack trace from a thrown error should be normalized and this really can be in some loop and being called infinite times. So, I think it's already up to you to decide which depth to set. As I understood 100 is enough, and I would simply keep it.

Should I create PR with setup for 0x? Flamegraphs doesn't show something specific in this case, but this setup might be useful in other cases.