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

Make condition queries faster #113

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tonistiigi
Copy link

Initial benchmark shows ~700x improvement.

It doesn't use sandbox so its not 100% compatible with previous version but I think its very hard to accidentally leak anything. On the other hand it now allows combining conditions for some complex queries.

I started with a version that uses new Function() instead of eval. tonistiigi/node-bunyan@master...condition-function In my opinion its cleaner but in this version condition needs to be a Javascript expression. So instead on if() you would have to use ?: etc. (dtrace users should be used to this). Anyway, it also turned out to be slower than eval. The slow part is the assignment of __proto__ for the log level definitions, without it its faster than eval.

Benchmark results:

Tested on a real log file with 1k/30k rows. Conditions matched for ~5% of the rows.

Original 1k rows: 12.557s flamegraph
Eval 30k rows: 0.449s flamegraph
Function 30k rows: 0.551s flamegraph
Eval no dunder proto 30k rows: 0.424s flamegraph
Function no dunder proto 30k rows: 0.390s flamegraph

@trentm
Copy link
Owner

trentm commented Aug 25, 2014

@tonistiigi Thanks for the spectacular analysis and I really apologize for the delay in responding.

I've implemented issue #87 in bunyan 1.0.0 using new Function. I need to look closer at your patch when I have the time again to consider eval. See also @nfitch's timing analysis of the equivalent -c ... open to my json tool at trentm/json#49

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

Successfully merging this pull request may close these issues.

2 participants