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

Refactor to use ffi #4

Closed
wants to merge 7 commits into from
Closed

Refactor to use ffi #4

wants to merge 7 commits into from

Conversation

bryanp
Copy link
Owner

@bryanp bryanp commented Dec 30, 2020

Replaces the MRI C extension with FFI.

This has not been tested extensively yet, and I'd like to do some benchmarking before committing to this direction.

@bryanp
Copy link
Owner Author

bryanp commented Dec 31, 2020

Right now the FFI implementation is considerably slower than the MRI C extension. Here's a benchmark for MRI:

Warming up --------------------------------------
                        57.981k i/100ms
Calculating -------------------------------------
                        630.524k (± 3.9%) i/s -      3.189M in   5.066022s

And here's the exact same benchmark for FFI:

Warming up --------------------------------------
                        10.349k i/100ms
Calculating -------------------------------------
                        104.139k (± 3.7%) i/s -    527.799k in   5.075701s

Should FFI really add this much overhead? I was hoping to see something within 10-20% of the MRI C extension. I'm not sure at this point if it's possible to optimize the FFI implementation or if this is more or less to be expected. One of my goals with this project is performance, so seeing this big of a difference between implementations is a bit disappointing.

What do you think @tarcieri?

@tarcieri
Copy link

tarcieri commented Dec 31, 2020

FFI trampolines through libffi which adds overhead, however that seems like a pretty severe degradation to me.

How many FFI calls is that making, and how much time is spent in each of those calls from a purely C perspective? Are there FFI calls that execute very quickly in C which can be consolidated in C to reduce the total number of FFI calls?

@bryanp
Copy link
Owner Author

bryanp commented Dec 31, 2020

I refactored a bit to do more in C and improved the FFI implementation from ~104k to ~180k. Better, but still not great.

65% of the time is spent in <Module::LLHttp>#llhttp_execute, which is the function that provides input to the parser. Since llhttp_execute is just an attached function and it seems plenty fast in the MRI C extension, all I can figure is that FFI is adding a lot of overhead there for some reason.

What I'm thinking right now is to keep llhttp as an MRI C extension and release llhttp-ffi as a separate gem. They'll both live in this repo and share specs so we know there's parity between them. That seems like the best of both worlds— performance for MRI and compatibility for JRuby.

@tarcieri
Copy link

tarcieri commented Jan 1, 2021

If you're willing to support both an MRI extension and FFI, that sounds great @bryanp.

That's an approach I'd never considered before, but if you wrap things up in the C/Ruby code nicely enough it may not be that much of a pain to do.

@bryanp
Copy link
Owner Author

bryanp commented Mar 4, 2021

Replaced by #12.

@bryanp bryanp closed this Mar 4, 2021
@bryanp bryanp deleted the chg/ffi branch March 4, 2021 04:40
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.

3 participants