Skip to content
This repository was archived by the owner on Feb 15, 2024. It is now read-only.

Replace libevent dependency with embedded libev#96

Closed
aido wants to merge 3 commits into
jgarzik:masterfrom
aido:libev
Closed

Replace libevent dependency with embedded libev#96
aido wants to merge 3 commits into
jgarzik:masterfrom
aido:libev

Conversation

@aido
Copy link
Copy Markdown
Contributor

@aido aido commented Dec 21, 2016

Hi,

This pull request proposes to replace the libevent dependency with directly embedded libev.
libev comes with a libevent compatibility API to make migration easier. Not all libevent functions are available in the API so event_new(), event_free() and event_base_loopbreak() had to be added to net.h. A few compiler warnings are generated but these can be ignored or removed if this pull request is acceptable.

The libev developers have the following to say about compiler warnings (https://linux.die.net/man/3/ev):

While libev is written to generate as few warnings as possible, "warn-free" code is not a goal, and it is recommended not to build libev with any compiler warnings enabled unless you are prepared to cope with them (e.g. by ignoring them). Remember that warnings are just that: warnings, not errors, or proof of bugs.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 76.852% when pulling 1e26920 on aido:libev into 3c3d504 on jgarzik:master.

1 similar comment
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 76.852% when pulling 1e26920 on aido:libev into 3c3d504 on jgarzik:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 76.852% when pulling f42ad4e on aido:libev into 3c3d504 on jgarzik:master.

1 similar comment
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 76.852% when pulling f42ad4e on aido:libev into 3c3d504 on jgarzik:master.

@jonasschnelli
Copy link
Copy Markdown
Contributor

jonasschnelli commented Dec 22, 2016

Would you be willing to elaborate the advantages of libev over libevent2?

@aido
Copy link
Copy Markdown
Contributor Author

aido commented Dec 22, 2016

Hi @jonasschnelli ,

The main advantage is the ease with which libev can be embedded, removing yet another libccoin dependency. libev is much smaller than libevent and if the benchmark results are the be believed libev is more efficient.
libev tries to do one thing only (POSIX event library), and this in the most efficient way possible. Libevent tries to give you the full solution (event lib, non-blocking I/O library, http server, DNS client).

This stackoverflow link explains some of the advantages of libev over libevent:
https://stackoverflow.com/questions/9433864/whats-the-difference-between-libev-and-libevent
Although, the description may be biased as it was written by the libev author 😄

libev may be a suitable solution for libbtc also, but on a separate issue have you seen a previous pull request #95? I see that you were considering writing a libfortunaprng library. Is any of the code used in #95 of use to you? It is based on a Postgres implementation of Fortuna and is a minimized (possibly improved) version of libfortuna (also based on Postgres). Maybe some of this code could be reused in libbtc.

@jonasschnelli
Copy link
Copy Markdown
Contributor

@aido Thanks for the great explanation.
Isn't the http server (maybe add RPC one) and the DNS client stuff (request ip's from seeds) useful?

fortuna:
Yes. Libfortuneprng was created to be used in Bitcoin-Core shortly after this PR bitcoin/bitcoin#5885.
IMO the PR above has some advantages over the one you try to include here.
Also, it would introduce MD5 into these sources (not sure if you want this).

What I'd like to do is porting the Core PR above to C to libfortuneprng (maybe a nice xmas project 🤓 ).

@jgarzik
Copy link
Copy Markdown
Owner

jgarzik commented Dec 22, 2016

The http stuff from libevent already caused much pain (through limitations) in Bitcoin Core, so I would prefer to avoid that again; prefer to just use the base event layer.

I'm agnostic on the base layer - libevent or libev - not really thrilled about including a system-ish lib in the picocoin codebase.

@jonasschnelli
Copy link
Copy Markdown
Contributor

The http stuff from libevent already caused much pain (through limitations) in Bitcoin Core, so I would prefer to avoid that again; prefer to just use the base event layer.

Yes. There are limitations. If you need high-scale httpd, it's probably the wrong tool.

not really thrilled about including a system-ish lib in the picocoin codebase.

I think the advantages of libevent are quite neat. Stuff like bandwidth limitation, etc. can be useful. But agree, it's another (cross)compile hassle.

@jgarzik
Copy link
Copy Markdown
Owner

jgarzik commented Dec 22, 2016

I think the advantages of libevent are quite neat. Stuff like bandwidth limitation, etc. can be useful. But agree, it's another (cross)compile hassle.

I was referring to importing a tree / adding a git subtree into the codebase. Would rather link to an external libevent or libev using the normal autoconf toolage, and avoid importing libev[ent] code directly into the picocoin build.

Using libev[ent] is good, yes. Much prefer to use libevent rather than direct select/poll coding.

@jgarzik
Copy link
Copy Markdown
Owner

jgarzik commented Dec 22, 2016

The http stuff from libevent already caused much pain (through limitations) in Bitcoin Core, so I would prefer to avoid that again; prefer to just use the base event layer.

Yes. There are limitations. If you need high-scale httpd, it's probably the wrong tool.

It's not about scale. The HTTP connection handling is very GET-or-form-POST centric, and was written before the era of bidirectional connections, push, PUT upload and many other modern advancements. These features are then tacked on poorly. I've written several servers using libevent's http and seen this up close. libevhtp is an illustration of the many areas where libevent http falls short. https://github.com/ellzey/libevhtp

@jonasschnelli
Copy link
Copy Markdown
Contributor

@jgarzik: thanks for pointing this out. TIL.

@aido
Copy link
Copy Markdown
Contributor Author

aido commented Dec 23, 2016

Hi,

OK, there are more pros than cons to leaving libevent as is.

Closing PR.

@aido aido closed this Dec 23, 2016
@aido aido deleted the libev branch December 28, 2016 23:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants