Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Possible benchmark performance regression and async benchmark segfaults #72

Open
Ingramz opened this issue Sep 22, 2017 · 3 comments
Open

Comments

@Ingramz
Copy link
Contributor

Ingramz commented Sep 22, 2017

I noticed an issue while trying to run benchmarks:
v5.1.2 with 898fc90 applied:

> .\node_modules\.bin\coffee .\benchmark\benchmark.coffee
oneline.js
sync:  149652 matches in 260ms
large.js
sync:  14594 matches in 69ms
async: 14594 matches in 795ms
> echo $LASTEXITCODE
0

v6.0.0 and up:

> .\node_modules\.bin\coffee .\benchmark\benchmark.coffee
OR (depending on version)
> node .\benchmark\benchmark.js
oneline.js
sync:  149652 matches in 21987ms
large.js
sync:  14594 matches in 243ms
> echo $LASTEXITCODE
-1073741819

Sometimes async gets displayed as well and everything looks "normal":

> .\node_modules\.bin\coffee .\benchmark\benchmark.coffee
oneline.js
sync:  149652 matches in 22129ms
large.js
sync:  14594 matches in 245ms
async: 14595 matches in 740ms
> echo $LASTEXITCODE
0

Same result on both OS X and Windows.

Whether the benchmark results are meaningful, I cannot tell, however I couldn't figure out what exactly causes the segfaults in async benchmark.

Pinging @alexandrudima @zcbenz @maxbrunsfeld

@michaelb87
Copy link

Hi,
I noticed those segfaults on a production app based on load.
When we were debugging this using gdb, we found it's due to a race condition of the async worker.

Unfortunately I don't have a fix, since we decided to just replace the library.

@Aerijo
Copy link

Aerijo commented Dec 14, 2019

The shared pointer holding the OnigResult appears to be the cause of the segfaults when it gets destructed (coupled with thread race stuff). Converting it to a regular pointer and managing memory manually seems to fix it. I'm not sure why the shared pointer is having issues though.

@michaelb87
Copy link

@Aerijo out of curiosity I'm revisiting this from time to time.
Found the pull request where they replaced the regular pointer with a shared pointer: https://github.com/atom/node-oniguruma/pull/6/files
Seems they fixed one issue, but might have introduced another one. Or the async stuff was added later, not sure.

I'm not a C++ dev, but might give it a shot on fixing this. If anyone has recommendations on how to approach the issue, I'm more than happy for input.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants