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 crash when importing big linux perf tool files #435

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

Goose97
Copy link
Contributor

@Goose97 Goose97 commented Jun 28, 2023

Currently, importing files generated by linux perf tool whose some blocks exceed V8 strings limit can crash the application. This issue is similar to the one in #385.

This PR fixes it by changing parseEvents to work directly with lines instead of chunking lines into blocks first.

Fixes #433

@coveralls
Copy link

coveralls commented Jun 28, 2023

Coverage Status

coverage: 42.952% (-0.06%) from 43.012% when pulling e1dacfe on Goose97:fix-large-file-crash into 26884c1 on jlfwong:main.

Copy link
Owner

@jlfwong jlfwong left a comment

Choose a reason for hiding this comment

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

Nice! A few things inline that need changing, then assume CI passes this should be good to go.

Also, just as a sanity check, this does now load the massive file you referenced in the issue, right?

src/import/linux-tools-perf.ts Outdated Show resolved Hide resolved
src/import/linux-tools-perf.ts Outdated Show resolved Hide resolved
src/import/linux-tools-perf.ts Outdated Show resolved Hide resolved
@jlfwong
Copy link
Owner

jlfwong commented Jun 28, 2023

I'm also curious how quickly such a large file loads, and what the performance bottlenecks of the load are

@Goose97
Copy link
Contributor Author

Goose97 commented Jun 28, 2023

Also, just as a sanity check, this does now load the massive file you referenced in the issue, right?

Yes, it works ok now

Currently, importing files generated by linux perf tool whose some blocks exceed V8 strings limit can crash the application. This issue is similar to the one in jlfwong#385.

This PR fixes it by changing parseEvents to work directly with lines instead of chunking lines into blocks first.

Fixes jlfwong#433
@jlfwong jlfwong merged commit 984bf12 into jlfwong:main Jun 28, 2023
8 checks passed
@jlfwong
Copy link
Owner

jlfwong commented Jun 28, 2023

Thank you!

@Goose97
Copy link
Contributor Author

Goose97 commented Jun 28, 2023

I'm also curious how quickly such a large file loads, and what the performance bottlenecks of the load are

Ironically, I just realize that it's not a linux perf tool file but a Brendan Gregg-format file. Oh silly me. The code just accidentally hits the error part and crashes.

Anw, it took ~23 seconds to import the file and took about ~1 second to render. Out of 23 seconds, ~2 seconds was reading the file into the buffer. The rest is parsing. I have the Chrome profiler dump but it's too large to attach to Github. Do you want to have a look?

System info:

  • Chrome version: 114.0.5735.133
  • OS: macOS Monterey 12.6.5
  • Memory: 16GB

@jlfwong
Copy link
Owner

jlfwong commented Jun 28, 2023

Ironically, I just realize that it's not a linux perf tool file but a Brendan Gregg-format file. Oh silly me. The code just accidentally hits the error part and crashes.

Ah jeez, I see -- for plaintext files with no known file extension, we always try importFromLinuxPerf before importFromBGFlameGraph.

If you can think of a way of avoiding that by doing some cheap heuristic detection on one format or the other, I'd take a PR to do that too.

To see if that's worthwhile for performance reasons, you could delete the lines that run importFromLinuxPerf, and try to import the massive file you're working with.

jlfwong pushed a commit that referenced this pull request Jun 29, 2023
Follow up on this PR #435. 

Currently, it took roughly 22 seconds to load my 1.3GB file. After inspecting the profiler, there's a large chunk of time spending in Frame.getOrInsert. I figure we can reduce the number of invocations by half. It reduces the load time to roughly 18 seconds.I also tested with a smaller file (~350MB), and it show similar gains, about 15-20%
jackerghan pushed a commit to jackerghan/speedscope that referenced this pull request Jul 28, 2023
Currently, importing files generated by linux perf tool whose some blocks exceed V8 strings limit can crash the application. This issue is similar to the one in jlfwong#385.

This PR fixes it by changing parseEvents to work directly with lines instead of chunking lines into blocks first.

Fixes jlfwong#433
jackerghan pushed a commit to jackerghan/speedscope that referenced this pull request Jul 28, 2023
Follow up on this PR jlfwong#435. 

Currently, it took roughly 22 seconds to load my 1.3GB file. After inspecting the profiler, there's a large chunk of time spending in Frame.getOrInsert. I figure we can reduce the number of invocations by half. It reduces the load time to roughly 18 seconds.I also tested with a smaller file (~350MB), and it show similar gains, about 15-20%
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.

Importing a large linux perf file will crash the application
3 participants