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

Fix sed command line in "Diagnostics - Flame Graphs" guide #4636

Merged
merged 4 commits into from
Mar 15, 2023

Conversation

kazarmy
Copy link
Contributor

@kazarmy kazarmy commented Jun 4, 2022

This pr fixes the sed command line used for perf file cleanup by enabling Extended Regular Expressions (ERE) syntax, so that the first sed script (that deletes lines containing " __libc_start", "LazyCompile" etc.) works. The alternative to enabling ERE syntax is to escape (, |, ) and ? which is less elegant.

@nschonni
Copy link
Member

nschonni commented Jun 4, 2022

Ping @nodejs/diagnostics

@kazarmy
Copy link
Contributor Author

kazarmy commented Jun 14, 2022

Hi, it's been more than a week. You can verify this fix yourself using diff -u on before/after perf files to see whether lines containing __libc_start| LazyCompile | v8::internal::| Builtin:| Stub:| LoadIC:|\[unknown\]| LoadPolymorphicIC: are actually deleted.

@ovflowd
Copy link
Member

ovflowd commented Mar 14, 2023

I can confirm that this works as intended. @kazarmy sorry for the extremely slow reply, feel free to rebase this PR and I would more than happy approve and merge it :)

But yet, @nodejs/diagnostics if y'all want to have a word here

@kazarmy
Copy link
Contributor Author

kazarmy commented Mar 14, 2023

@kazarmy sorry for the extremely slow reply

No problem ... better late than never :)

feel free to rebase this PR

Ok done

@tony-go
Copy link
Member

tony-go commented Mar 14, 2023

I didn't have time to test it at the moment. I'll try to do it this week.

@ovflowd ovflowd merged commit 48b7b1f into nodejs:main Mar 15, 2023
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.

5 participants