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

Added native event support #814

Merged
merged 8 commits into from Dec 20, 2014

Conversation

Projects
None yet
8 participants
@etcimon
Contributor

etcimon commented Sep 9, 2014

This is currently ready for testing. I'm in the process of adding DirectoryWatcher and FileDescriptor but didn't think it was necessary for now.

I've tested the http_server example, and the message example.

This is only tested for DMD 2.066 on my end, but Linux/Windows/MacOS are working and I haven't tried BSD but it should be working too.

The HTTP Server example size is about 1.59MB vs 1.41MB+(libevent: 359KB) on windows, and 4.79MB vs 4.54MB+(libevent: 0.295MB+0.179MB) on linux, though I haven't benchmarked it yet. You might want to remove debugging by making event.type.LOG = false in event-d type.d

Also, the ws2_32.lib file is wrong in DMD, so I included one but I'm not sure if it gets carried through the linking stage correctly with my current dub configuration.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 10, 2014

Contributor

The error seems to be with the RedisSubscriber module.

Contributor

etcimon commented Sep 10, 2014

The error seems to be with the RedisSubscriber module.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 18, 2014

Contributor

Does dub support receiving a specific version based on the one in the dependencies? It fetches event-d ~master but then says it doesn't match "exactly" the version 0.5.1, I would have expected it to fetch the tag v0.5.1 in the first place?

Contributor

etcimon commented Sep 18, 2014

Does dub support receiving a specific version based on the one in the dependencies? It fetches event-d ~master but then says it doesn't match "exactly" the version 0.5.1, I would have expected it to fetch the tag v0.5.1 in the first place?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 18, 2014

Member

Sounds strange, maybe the version tag hasn't been online yet during the first fetch? What happens for dub upgrade?

Member

s-ludwig commented Sep 18, 2014

Sounds strange, maybe the version tag hasn't been online yet during the first fetch? What happens for dub upgrade?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 18, 2014

Contributor

dub fetch event-d fetches v0.4.2 which is outdated, I tried removing the package though

Contributor

etcimon commented Sep 18, 2014

dub fetch event-d fetches v0.4.2 which is outdated, I tried removing the package though

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 18, 2014

Member

For me it seems to work fine:

$ dub fetch event-d
Fetching event-d 0.5.1...

Does dub --vverbose print anything that could explain the difference?

Member

s-ludwig commented Sep 18, 2014

For me it seems to work fine:

$ dub fetch event-d
Fetching event-d 0.5.1...

Does dub --vverbose print anything that could explain the difference?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 23, 2014

Contributor

Whew, this is done. Well, done, meaning I'm ready to use this without committing any breaking changes now =) And it really has all the features you've laid out that a driver should provide, and I've tried as many examples I could with it, on linux & windows & mac os x. Would you like me to merge all the commits as a single one? Btw, this pull request holds the Redis changes from the other pull request, because they were making travis fail.

I'm going to make a new ssl stream based on botan now. After that, I'll implement a HTTP2Client and HTTP2Server. The most exciting will follow this one: a clustered web filesystem that works a little like a key-value store but using directories and integrated ACL (same concept as windows NTFS permissions). I'm abandoning somewhat the cache database idea in favor of a small-data optimized virtual filesystem that runs with the Higgs virtual machine for ".js" executable files stored inside it, and embeds with the HTTP2 router for browser/json-based API access that allows it to behave like a CMS from the web developer's point of view but with the robustness of an operating system.

Contributor

etcimon commented Sep 23, 2014

Whew, this is done. Well, done, meaning I'm ready to use this without committing any breaking changes now =) And it really has all the features you've laid out that a driver should provide, and I've tried as many examples I could with it, on linux & windows & mac os x. Would you like me to merge all the commits as a single one? Btw, this pull request holds the Redis changes from the other pull request, because they were making travis fail.

I'm going to make a new ssl stream based on botan now. After that, I'll implement a HTTP2Client and HTTP2Server. The most exciting will follow this one: a clustered web filesystem that works a little like a key-value store but using directories and integrated ACL (same concept as windows NTFS permissions). I'm abandoning somewhat the cache database idea in favor of a small-data optimized virtual filesystem that runs with the Higgs virtual machine for ".js" executable files stored inside it, and embeds with the HTTP2 router for browser/json-based API access that allows it to behave like a CMS from the web developer's point of view but with the robustness of an operating system.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 23, 2014

Member

Sounds like an interesting project... it would be good to think about ways to nicely integrate HTTP/1.x and HTTP/2.x into a single client/server. I think it should be possible to at least put everything behind the existing interface, even if it ends up necessary to implement 2.0 using a mostly separate code base.

I'll try to review the changes ASAP, but I can't make promises currently. One thing I've noticed is that the driver should be in event.d or eventd.d, rather than native.d. Until we know that it's stable, it should also stay as an option (-version=VibeEventDDriver).

BTW, OT, do you think there is a nice unique name for event.d that works without the "d" part? Recently it struck my eye that so many D libraries explicitly mention "D" in their names, while I think that we should emancipate ourselves from that and simply name things after what they do instead of in which language they are written in. Today I'd probably drop the .d part of "vibe.d", or maybe use "vibed", but unfortunately the vibe. package name space is already settled.

Member

s-ludwig commented Sep 23, 2014

Sounds like an interesting project... it would be good to think about ways to nicely integrate HTTP/1.x and HTTP/2.x into a single client/server. I think it should be possible to at least put everything behind the existing interface, even if it ends up necessary to implement 2.0 using a mostly separate code base.

I'll try to review the changes ASAP, but I can't make promises currently. One thing I've noticed is that the driver should be in event.d or eventd.d, rather than native.d. Until we know that it's stable, it should also stay as an option (-version=VibeEventDDriver).

BTW, OT, do you think there is a nice unique name for event.d that works without the "d" part? Recently it struck my eye that so many D libraries explicitly mention "D" in their names, while I think that we should emancipate ourselves from that and simply name things after what they do instead of in which language they are written in. Today I'd probably drop the .d part of "vibe.d", or maybe use "vibed", but unfortunately the vibe. package name space is already settled.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 23, 2014

Contributor

BTW, OT, do you think there is a nice unique name for event.d that works without the "d" part?

Sure, my priority has always been consistency so I never gave it a thought. I like the name libasync, and it looks like any code or libraries using the name are outdated by a decade or more. I'll rename native.d => libasync.d

Sounds like an interesting project... it would be good to think about ways to nicely integrate HTTP/1.x and HTTP/2.x into a single client/server. I think it should be possible to at least put everything behind the existing interface, even if it ends up necessary to implement 2.0 using a mostly separate code base.

The interface would indeed be the same, the only difference would be an added Task[StreamID] m_waiters hashmap for waiting on replies, with a nice websocket-type envelope for messages . While the callback would have to take an HTTP2ServerRequest / HTTP2ServerResponse object (with an added stream ID member), it would simply proxy to the HTTPServerRequest/... if HTTP2 is not available. The parser would be completely different in connections where HTTP2 is available.

Contributor

etcimon commented Sep 23, 2014

BTW, OT, do you think there is a nice unique name for event.d that works without the "d" part?

Sure, my priority has always been consistency so I never gave it a thought. I like the name libasync, and it looks like any code or libraries using the name are outdated by a decade or more. I'll rename native.d => libasync.d

Sounds like an interesting project... it would be good to think about ways to nicely integrate HTTP/1.x and HTTP/2.x into a single client/server. I think it should be possible to at least put everything behind the existing interface, even if it ends up necessary to implement 2.0 using a mostly separate code base.

The interface would indeed be the same, the only difference would be an added Task[StreamID] m_waiters hashmap for waiting on replies, with a nice websocket-type envelope for messages . While the callback would have to take an HTTP2ServerRequest / HTTP2ServerResponse object (with an added stream ID member), it would simply proxy to the HTTPServerRequest/... if HTTP2 is not available. The parser would be completely different in connections where HTTP2 is available.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 23, 2014

Member

I'll rename native.d => libasync.d

That would definitely be a very prominent name

HTTP2ServerRequest / HTTP2ServerReply

I think we should rather just keep HTTPServerRequest/Response and add the stream id member there (unused for HTTP/1.x) connections. It most probably won't pay off to introduce separate types there.

Member

s-ludwig commented Sep 23, 2014

I'll rename native.d => libasync.d

That would definitely be a very prominent name

HTTP2ServerRequest / HTTP2ServerReply

I think we should rather just keep HTTPServerRequest/Response and add the stream id member there (unused for HTTP/1.x) connections. It most probably won't pay off to introduce separate types there.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 23, 2014

Contributor

It most probably won't pay off to introduce separate types there.

Yes, it would probably be better to have a separate class instantiated inside the object and make the HTTP* classes proxy through there after HTTP2 is known available for a connection.

Contributor

etcimon commented Sep 23, 2014

It most probably won't pay off to introduce separate types there.

Yes, it would probably be better to have a separate class instantiated inside the object and make the HTTP* classes proxy through there after HTTP2 is known available for a connection.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 23, 2014

Contributor

I'll keep it default to make sure travis tests it correctly, and will make it optional once you've agree to merge.

Contributor

etcimon commented Sep 23, 2014

I'll keep it default to make sure travis tests it correctly, and will make it optional once you've agree to merge.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 23, 2014

Contributor

Ok, I did a few adjustments and I'm pretty sure it's ready. Make sure you update the v0.6.0, I had to overwrite under the same tag after I noticed a collision between vibe.core.concurrency.async and the async.* namespace I was using. I left out the Redis changes (although I'm leaving the sleep() calls where they're needed for this library to pass travis).

Contributor

etcimon commented Sep 23, 2014

Ok, I did a few adjustments and I'm pretty sure it's ready. Make sure you update the v0.6.0, I had to overwrite under the same tag after I noticed a collision between vibe.core.concurrency.async and the async.* namespace I was using. I left out the Redis changes (although I'm leaving the sleep() calls where they're needed for this library to pass travis).

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 25, 2014

Contributor

I had an invitation by Andrei to put libasync in the std.async namespace. The biggest work will be in std.concurrency with the threads communicating together through the event loop optionally. I'm wondering if you think this is a good opportunity to merge the vibe.d tasks / streams in there as well?

Contributor

etcimon commented Sep 25, 2014

I had an invitation by Andrei to put libasync in the std.async namespace. The biggest work will be in std.concurrency with the threads communicating together through the event loop optionally. I'm wondering if you think this is a good opportunity to merge the vibe.d tasks / streams in there as well?

@ColdenCullen

This comment has been minimized.

Show comment
Hide comment
@ColdenCullen

ColdenCullen Sep 25, 2014

Contributor

The sooner we can get this merged, the happier I'll be. Looks currently this is the only way to use a directory watcher on Windows.

Contributor

ColdenCullen commented Sep 25, 2014

The sooner we can get this merged, the happier I'll be. Looks currently this is the only way to use a directory watcher on Windows.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 25, 2014

Member

@ColdenCullen: The win32 driver (--config=win32) is supposed to provide a working directory watcher, or is that broken?

Member

s-ludwig commented Sep 25, 2014

@ColdenCullen: The win32 driver (--config=win32) is supposed to provide a working directory watcher, or is that broken?

@ColdenCullen

This comment has been minimized.

Show comment
Hide comment
@ColdenCullen

ColdenCullen Sep 25, 2014

Contributor

It does, but it doesn't support a 0/positive timeout, which is what I need..

Contributor

ColdenCullen commented Sep 25, 2014

It does, but it doesn't support a 0/positive timeout, which is what I need..

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 25, 2014

Member

Wouldn't running readChanges in a separate task be a workaround for that?

Anyway, I need some time to do a proper review for this PR, but I'm still overloaded with other things and am currently only able to deal with small tasks. But it's at the top of the priority list otherwise.

Member

s-ludwig commented Sep 25, 2014

Wouldn't running readChanges in a separate task be a workaround for that?

Anyway, I need some time to do a proper review for this PR, but I'm still overloaded with other things and am currently only able to deal with small tasks. But it's at the top of the priority list otherwise.

@ColdenCullen

This comment has been minimized.

Show comment
Hide comment
@ColdenCullen

ColdenCullen Sep 25, 2014

Contributor

Yes, I suppose it would. Although I was hoping to avoid that if at all possible. Also, is there any way in dub currently to only use the win32 sub-configuration only on Windows? I'd like to not break Linux compatibility.

Contributor

ColdenCullen commented Sep 25, 2014

Yes, I suppose it would. Although I was hoping to avoid that if at all possible. Also, is there any way in dub currently to only use the win32 sub-configuration only on Windows? I'd like to not break Linux compatibility.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 26, 2014

Member

That currently requires using a separate configuration:

{
  "dependencies": {
    "vibe-d": "~>0.7.20"
  },
  "configurations": [
    {
      "name": "win32",
      "platforms": ["windows"],
      "subConfigurations": {"vibe-d": "win32"}
    },
    {
      "name": "default"
    }
  ]
}
Member

s-ludwig commented Sep 26, 2014

That currently requires using a separate configuration:

{
  "dependencies": {
    "vibe-d": "~>0.7.20"
  },
  "configurations": [
    {
      "name": "win32",
      "platforms": ["windows"],
      "subConfigurations": {"vibe-d": "win32"}
    },
    {
      "name": "default"
    }
  ]
}
@ColdenCullen

This comment has been minimized.

Show comment
Hide comment
@ColdenCullen

ColdenCullen Sep 26, 2014

Contributor

That's not exactly as attractive as I was hoping it would be. I think I'll just hold off until libasync is in.

Contributor

ColdenCullen commented Sep 26, 2014

That's not exactly as attractive as I was hoping it would be. I think I'll just hold off until libasync is in.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 27, 2014

Member

Okay, I have mostly only formal stuff. There are a few changes that are unrelated and that would be better to do in separate pull requests (or at least commits):

  • examples/http_server/source/app.d
  • examples/serialization/dub.json
  • source/vibe/core/log.d
  • source/vibe/core/core.d
  • tests/mongodb/dub.json
  • tests/redis/dub.json
  • tests/redis/source/app.d

Except in vibe.core.core the Thread.getThis().name == "CmdProcessor" test is related of course. But this needs a more generic or a more contained solution. Maybe setupEventDriver can simply add something like static if (is(typeof(NativeEventDriver.isControlThread))) if (NativeEventDriver.isControlThread) return false; and then the LibasyncDriver can properly define that method.

These modules that are currently public should be wrapped where necessary instead. We can deprecate vibe.d's versions of those once we decide that libasync is the only necessary back end.

  • libasync.internals.memory
  • libasync.internals.path
  • libasync.events : NetworkAddress

In the case of libasync.internals.path, I would recommend to remove that from libasync altogether, especially considering that it aims for Phobos inclusion (Walter strongly objected against a Path type, which I think is silly, but anyway). A simple string based API should suffice on that low level. NetworkAddress would of course also have to be merged into std.socket for Phobos inclusion. I guess that could be done nicely as a preparatory pull request to reduce the impact of the actual std.async PR.

In the LibasyncDriver, instead of the gs_availID arrray, a simple long that is incremented should be much more efficient, or are IDs supposed to stay small integers?

Other than that, everything looked fine so far. Great to see this happen!

Member

s-ludwig commented Sep 27, 2014

Okay, I have mostly only formal stuff. There are a few changes that are unrelated and that would be better to do in separate pull requests (or at least commits):

  • examples/http_server/source/app.d
  • examples/serialization/dub.json
  • source/vibe/core/log.d
  • source/vibe/core/core.d
  • tests/mongodb/dub.json
  • tests/redis/dub.json
  • tests/redis/source/app.d

Except in vibe.core.core the Thread.getThis().name == "CmdProcessor" test is related of course. But this needs a more generic or a more contained solution. Maybe setupEventDriver can simply add something like static if (is(typeof(NativeEventDriver.isControlThread))) if (NativeEventDriver.isControlThread) return false; and then the LibasyncDriver can properly define that method.

These modules that are currently public should be wrapped where necessary instead. We can deprecate vibe.d's versions of those once we decide that libasync is the only necessary back end.

  • libasync.internals.memory
  • libasync.internals.path
  • libasync.events : NetworkAddress

In the case of libasync.internals.path, I would recommend to remove that from libasync altogether, especially considering that it aims for Phobos inclusion (Walter strongly objected against a Path type, which I think is silly, but anyway). A simple string based API should suffice on that low level. NetworkAddress would of course also have to be merged into std.socket for Phobos inclusion. I guess that could be done nicely as a preparatory pull request to reduce the impact of the actual std.async PR.

In the LibasyncDriver, instead of the gs_availID arrray, a simple long that is incremented should be much more efficient, or are IDs supposed to stay small integers?

Other than that, everything looked fine so far. Great to see this happen!

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 27, 2014

Contributor

Thanks for the quick review. I think the Path is better kept private if it can't be in the interface, its very helpful.

The gs_availID is a nice trick I found for making temporary auto increment IDs on small pools of frequently disposed objects e.g. for signals on kqueue, and I thought it would be useful in this case as well to make manual events lighter, while avoiding a collision if it wraps around.

I will start dividing the pull request and applying those changes, I'm afraid that if I submit this to phobos without tasks we will have libraries coming to life using futures rather than streams. Do you have any idea w.r.t avoiding this? I'm assuming putting streams in std.vibe would discourage this

Contributor

etcimon commented Sep 27, 2014

Thanks for the quick review. I think the Path is better kept private if it can't be in the interface, its very helpful.

The gs_availID is a nice trick I found for making temporary auto increment IDs on small pools of frequently disposed objects e.g. for signals on kqueue, and I thought it would be useful in this case as well to make manual events lighter, while avoiding a collision if it wraps around.

I will start dividing the pull request and applying those changes, I'm afraid that if I submit this to phobos without tasks we will have libraries coming to life using futures rather than streams. Do you have any idea w.r.t avoiding this? I'm assuming putting streams in std.vibe would discourage this

@s-ludwig s-ludwig added needs input and removed needs review labels Sep 28, 2014

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 28, 2014

Contributor

I plan to add some compatible futures and integrate them in the driver so the std.vibe shouldn't be necessary here. The key is to have a global wait delegate, and to be able to use Future.wait() that will run the event loop by default, and vibe can replace the delegate with an implementation that composes a manual event and context switch.

Contributor

etcimon commented Sep 28, 2014

I plan to add some compatible futures and integrate them in the driver so the std.vibe shouldn't be necessary here. The key is to have a global wait delegate, and to be able to use Future.wait() that will run the event loop by default, and vibe can replace the delegate with an implementation that composes a manual event and context switch.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Sep 28, 2014

Contributor

One question: Would you see the event loop in EventLoop.getThis() ? It makes no sense to have more than an event loop per thread and this would significantly simplify things.

Contributor

etcimon commented Sep 28, 2014

One question: Would you see the event loop in EventLoop.getThis() ? It makes no sense to have more than an event loop per thread and this would significantly simplify things.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 2, 2014

Contributor

These modules that are currently public should be wrapped where necessary instead. We can deprecate vibe.d's versions of those once we decide that libasync is the only necessary back end.

I'm not sure how I would go about doing this? Do you mean sending the Vibe ones through template parameters?

Contributor

etcimon commented Oct 2, 2014

These modules that are currently public should be wrapped where necessary instead. We can deprecate vibe.d's versions of those once we decide that libasync is the only necessary back end.

I'm not sure how I would go about doing this? Do you mean sending the Vibe ones through template parameters?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 2, 2014

Contributor

Otherwise, I'd consider this ready

Contributor

etcimon commented Oct 2, 2014

Otherwise, I'd consider this ready

@etcimon etcimon referenced this pull request Oct 21, 2014

Closed

Is libasync stable? #5

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 31, 2014

Contributor

Is there anything left to add before this can be merged?

Contributor

etcimon commented Oct 31, 2014

Is there anything left to add before this can be merged?

@GeorgeSapkin

This comment has been minimized.

Show comment
Hide comment
@GeorgeSapkin

GeorgeSapkin Nov 18, 2014

Contributor

This has been tested and working on Windows 32 & 64 bit with latest libasync and druntime masters. What is the status of this PR?

Contributor

GeorgeSapkin commented Nov 18, 2014

This has been tested and working on Windows 32 & 64 bit with latest libasync and druntime masters. What is the status of this PR?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Nov 18, 2014

Contributor

I've been using it live on a heavy-traffic e-commerce. It's stable for me

Contributor

etcimon commented Nov 18, 2014

I've been using it live on a heavy-traffic e-commerce. It's stable for me

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 18, 2014

Member

Apart from "examples/http_server/dub.json" (shouldn't be included in the PR), the module constructor workaround (need to take a closer look) and the public imports of libasync, this is good to go. I can take over the last point after the merge, though.

Member

s-ludwig commented Nov 18, 2014

Apart from "examples/http_server/dub.json" (shouldn't be included in the PR), the module constructor workaround (need to take a closer look) and the public imports of libasync, this is good to go. I can take over the last point after the merge, though.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Dec 20, 2014

Member

Going to merge now, before this takes forever. I'll create a separate issue to remember fixing the open issues.

Member

s-ludwig commented Dec 20, 2014

Going to merge now, before this takes forever. I'll create a separate issue to remember fixing the open issues.

s-ludwig added a commit that referenced this pull request Dec 20, 2014

Merge pull request #814 from etcimon/native-events
Add native event support based on libasync

@s-ludwig s-ludwig merged commit 8b9f556 into vibe-d:master Dec 20, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
@socmag

This comment has been minimized.

Show comment
Hide comment
@socmag

socmag Dec 20, 2014

great, thanks!
Merry Christmas

socmag commented Dec 20, 2014

great, thanks!
Merry Christmas

@mihails-strasuns

This comment has been minimized.

Show comment
Hide comment
@mihails-strasuns

mihails-strasuns Dec 20, 2014

Contributor

@etcimon do you have any benchmarks for this against libevent btw?

Contributor

mihails-strasuns commented Dec 20, 2014

@etcimon do you have any benchmarks for this against libevent btw?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Dec 20, 2014

Contributor

@etcimon do you have any benchmarks for this against libevent btw?

It's 20-25% slower when you compile with DMD/release. However, libevent is compiled with GCC, and DMD is known to produce less efficient machine code compared to that. My focus from the start was easier compilation, standalone executable packaging, and better coupling with vibe.d, so it serves those purposes. For libasync, there should be no compilation issues with GDC or LDC but a couple months ago I couldn't get vibe.d to compile with it, so I'm holding off for later.

Sorry for not better integrating it to vibe.d's containers and memory. I think a Have_vibe_d workaround could've worked, and I've been meaning to do it but I decided to do some research on containers to better support FreeLists and RAII, which I'm currently doing in my Botan project.

So far, I'll have a (maybe working) ref counting, nogc Red Black Tree, std.Array renamed to Vector, a more versatile FreeListRef that sends through the op____ operator overloads, observes const-ness, and finally a few newly templated memory containers including one that zeroises the memory is holds.

It's still just a brainstorm, but sending a ref counted Vector like C++ through the streams is a good way to further avoid the garbage collector in those streams, because the allocator can be used to push the data in it. Also, I intended to re-write the timer algorithm in Libasync, which is why I'm working on the rbtree.

This will be tackled once I'm through with implementing the BotanTLSStream. For now the Botan library compiles 90% (until the SIMD part) and is aimed at all 3 compilers

Contributor

etcimon commented Dec 20, 2014

@etcimon do you have any benchmarks for this against libevent btw?

It's 20-25% slower when you compile with DMD/release. However, libevent is compiled with GCC, and DMD is known to produce less efficient machine code compared to that. My focus from the start was easier compilation, standalone executable packaging, and better coupling with vibe.d, so it serves those purposes. For libasync, there should be no compilation issues with GDC or LDC but a couple months ago I couldn't get vibe.d to compile with it, so I'm holding off for later.

Sorry for not better integrating it to vibe.d's containers and memory. I think a Have_vibe_d workaround could've worked, and I've been meaning to do it but I decided to do some research on containers to better support FreeLists and RAII, which I'm currently doing in my Botan project.

So far, I'll have a (maybe working) ref counting, nogc Red Black Tree, std.Array renamed to Vector, a more versatile FreeListRef that sends through the op____ operator overloads, observes const-ness, and finally a few newly templated memory containers including one that zeroises the memory is holds.

It's still just a brainstorm, but sending a ref counted Vector like C++ through the streams is a good way to further avoid the garbage collector in those streams, because the allocator can be used to push the data in it. Also, I intended to re-write the timer algorithm in Libasync, which is why I'm working on the rbtree.

This will be tackled once I'm through with implementing the BotanTLSStream. For now the Botan library compiles 90% (until the SIMD part) and is aimed at all 3 compilers

@dariusc93

This comment has been minimized.

Show comment
Hide comment
@dariusc93

dariusc93 Dec 21, 2014

Contributor

Cool that this got merged. Does this compiled against LDC or GDC without any issues?

Contributor

dariusc93 commented Dec 21, 2014

Cool that this got merged. Does this compiled against LDC or GDC without any issues?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Dec 21, 2014

Member

It still produced some errors (Travis-CI currently doesn't test the libasync configuration):

Member

s-ludwig commented Dec 21, 2014

It still produced some errors (Travis-CI currently doesn't test the libasync configuration):

@dariusc93

This comment has been minimized.

Show comment
Hide comment
@dariusc93
Contributor

dariusc93 commented Dec 21, 2014

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Dec 21, 2014

Contributor

This is pretty far away on my priority list, I'm currently focusing on features & idioms rather than optimizations so I'd probably encourage someone else to debug this.

Contributor

etcimon commented Dec 21, 2014

This is pretty far away on my priority list, I'm currently focusing on features & idioms rather than optimizations so I'd probably encourage someone else to debug this.

@jgoett154

This comment has been minimized.

Show comment
Hide comment
@jgoett154

jgoett154 Dec 21, 2014

That error occurs due to this DMD PR dlang/dmd#2924
Anything based on DMD < 2.066 will have that problem. Easiest and best solution is to move the UDAs to the beginning of the line.

jgoett154 commented Dec 21, 2014

That error occurs due to this DMD PR dlang/dmd#2924
Anything based on DMD < 2.066 will have that problem. Easiest and best solution is to move the UDAs to the beginning of the line.

@dariusc93

This comment has been minimized.

Show comment
Hide comment
@dariusc93

dariusc93 Dec 21, 2014

Contributor

@legobear154 if thats the case, @s-ludwig should try against 0.15.1 since thats base on 2.066

Contributor

dariusc93 commented Dec 21, 2014

@legobear154 if thats the case, @s-ludwig should try against 0.15.1 since thats base on 2.066

@jgoett154

This comment has been minimized.

Show comment
Hide comment
@jgoett154

jgoett154 commented Dec 21, 2014

@dariusc93

This comment has been minimized.

Show comment
Hide comment
@dariusc93

dariusc93 Dec 21, 2014

Contributor

@legobear154 Didnt see the 0.15.1 part in travis.

Contributor

dariusc93 commented Dec 21, 2014

@legobear154 Didnt see the 0.15.1 part in travis.

@jgoett154

This comment has been minimized.

Show comment
Hide comment
@jgoett154

jgoett154 Dec 21, 2014

Easy mistake :) Another problem (after moving the UDAs) is that GDC doesn't know about @nogc (and I would assume anything <2.066 would fail as well). So, a fix for that is necessary too. I forget the fix I have seen people use before, soon as I find it and get everything working I can submit a PR.

Quick edit: Might be harder to get working as some things apparently are nothrow in 2.066 but where not in <2.066

I doubt it will ever work on < 2.066 since @etcimon wrote it specifically for >=2.066. Might be best to add a version check to vibe.d and fail gracefully saying libasync doesn't support <2.066. Or to use the default if libasync was selected and the version is <2.066 as well as loudly yell at the person when building that libasync doesn't support <2.066 and that it is defaulting to whatever the default may be.

jgoett154 commented Dec 21, 2014

Easy mistake :) Another problem (after moving the UDAs) is that GDC doesn't know about @nogc (and I would assume anything <2.066 would fail as well). So, a fix for that is necessary too. I forget the fix I have seen people use before, soon as I find it and get everything working I can submit a PR.

Quick edit: Might be harder to get working as some things apparently are nothrow in 2.066 but where not in <2.066

I doubt it will ever work on < 2.066 since @etcimon wrote it specifically for >=2.066. Might be best to add a version check to vibe.d and fail gracefully saying libasync doesn't support <2.066. Or to use the default if libasync was selected and the version is <2.066 as well as loudly yell at the person when building that libasync doesn't support <2.066 and that it is defaulting to whatever the default may be.

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