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: improve speed 15x by using @indutny/breakpad #57

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

indutny
Copy link
Contributor

@indutny indutny commented Jun 14, 2024

Before:

$ npm test
Test Suites: 2 passed, 2 total
Tests:       7 passed, 7 total
Snapshots:   4 passed, 4 total
Time:        48.887 s

After:

$ npm test
Test Suites: 2 passed, 2 total
Tests:       7 passed, 7 total
Snapshots:   4 passed, 4 total
Time:        3.186 s

@indutny/breakpad also provides file/line number information, but I didn't want to make changes to the output format. Let me know if you want to add them in!

cc @nornagon

@indutny indutny requested review from a team as code owners June 14, 2024 17:20
@indutny indutny force-pushed the feature/speed branch 4 times, most recently from 585412a to afe1ee6 Compare June 14, 2024 17:41
}

const frames = group.map(({ offset }) => offset);
const symbolicated = await symbolicateFrames(stream, frames);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case you are wondering, this module streams once through the .sym file without building an index - you have to supply all the offsets that you want to identify right at the call time. Thus we have to group offsets by module first and then symbolicate them module-by-module.

On a good note, this is faster than building an AVL tree, and only an O(N) amount of memory where N is the number of frames.

const dumpText = await fs.promises.readFile(file, 'utf8')
const images = binaryImages(dumpText)
const electronImage = images.find(v => /electron/i.test(v.library))
if (electronImage) console.error(`Found Electron ${electronImage.version}`)
else console.error('No Electron image found')

let result = []
const lines = dumpText.split(/\r?\n/);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've decided to just modify this array in-place instead of rebuilding it from scratch. It is faster, but more importantly I could do a bunch of early continues in the loop and the code is not as deep anymore.

const image = {
startAddress: parseInt(startAddress, 16),
endAddress: parseInt(endAddress, 16),
plus,
library,
version,
debugId,
path
basename: path.basename(modulePath),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just so that we won't have to call them repeatedly all around the place.

@indutny indutny changed the title fix: improve speed 6x by using @indutny/breakpad fix: improve speed 15x by using @indutny/breakpad Jun 17, 2024
Before:
```
$ npm test
Test Suites: 2 passed, 2 total
Tests:       7 passed, 7 total
Snapshots:   4 passed, 4 total
Time:        48.887 s
```

After:
```
$ npm test
Test Suites: 2 passed, 2 total
Tests:       7 passed, 7 total
Snapshots:   4 passed, 4 total
Time:        3.186 s
```
@indutny-signal
Copy link

Friendly ping @nornagon ?

@indutny-signal
Copy link

indutny-signal commented Jul 1, 2024

🤗 (keeping this up in the inbox!)

@indutny-signal
Copy link

Never give up :)

@indutny-signal
Copy link

🥹

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