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

hide SSL implementation headers behind interfaces #757

Merged
merged 2 commits into from Jul 31, 2014

Conversation

Projects
None yet
3 participants
@MartinNowak
Contributor

MartinNowak commented Jul 30, 2014

  • turn SSLStream and SSLContext into interfaces
  • move OpenSSL based implementation to vibe.stream.openssl
hide SSL implementation headers behind interfaces
- turn SSLStream and SSLContext into interfaces

- move OpenSSL based implementation to vibe.stream.openssl
@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Jul 30, 2014

Contributor

Please review carefully, I just did this very mechanically.
It would probably make sense to turn SSLContext into a class instead of an interface as some methods should apply to any SSL implementation. Then those function could be made final methods.

This time I do have numbers. The raw compile time as measured here went down from 0.8s to 0.4s. Number of imported files was reduced from 202 to 152, LOC went from 222.5K to 192.7K and code size dropped from 8M to 6.8M.
It's a little surprising to see that the compile time reduced overproportional.

Contributor

MartinNowak commented Jul 30, 2014

Please review carefully, I just did this very mechanically.
It would probably make sense to turn SSLContext into a class instead of an interface as some methods should apply to any SSL implementation. Then those function could be made final methods.

This time I do have numbers. The raw compile time as measured here went down from 0.8s to 0.4s. Number of imported files was reduced from 202 to 152, LOC went from 222.5K to 192.7K and code size dropped from 8M to 6.8M.
It's a little surprising to see that the compile time reduced overproportional.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 31, 2014

Contributor

It's a little surprising to see that the compile time reduced overproportional.

So, this means there could be a use to a pragma-based compile-time profiler? :-P

Best I could find is this: http://forum.dlang.org/thread/ho9r0q$2doe$1@digitalmars.com

Contributor

etcimon commented Jul 31, 2014

It's a little surprising to see that the compile time reduced overproportional.

So, this means there could be a use to a pragma-based compile-time profiler? :-P

Best I could find is this: http://forum.dlang.org/thread/ho9r0q$2doe$1@digitalmars.com

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Jul 31, 2014

Contributor

Updated graph (http://jsfiddle.net/DJqjD/), the next victim will be dub :).
chart 1
There is probably still some potential to reduce phobos/druntime imports.

Contributor

MartinNowak commented Jul 31, 2014

Updated graph (http://jsfiddle.net/DJqjD/), the next victim will be dub :).
chart 1
There is probably still some potential to reduce phobos/druntime imports.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Jul 31, 2014

Contributor

So, this means there could be a use to a pragma-based compile-time profiler? :-P

From the old and the new perf histogram one can see, that the compiler has to do a huge amount of symbol lookups for openssl.
Even when a module is only imported the compiler will the first semantic pass on it.
It might be interesting to analyse this a bit more. Maybe there are a lot of cross-module dependencies in the openssl headers or it's all those ExternC templates that slow down the compiler.

Contributor

MartinNowak commented Jul 31, 2014

So, this means there could be a use to a pragma-based compile-time profiler? :-P

From the old and the new perf histogram one can see, that the compiler has to do a huge amount of symbol lookups for openssl.
Even when a module is only imported the compiler will the first semantic pass on it.
It might be interesting to analyse this a bit more. Maybe there are a lot of cross-module dependencies in the openssl headers or it's all those ExternC templates that slow down the compiler.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 31, 2014

Member

Apart from the missing const, everything looks fine.

Looks like an impressive compile time reduction for one small component. But judging be the monster that the OpenSSL API is, its actually not surprising ;)

BTW, Vladimir also made this useful little tool that would split up the build time into its components (that he also demonstrated on DConf): DBuildStat - that gave some interesting insights in an earlier attempt to reduce compile times.

Member

s-ludwig commented Jul 31, 2014

Apart from the missing const, everything looks fine.

Looks like an impressive compile time reduction for one small component. But judging be the monster that the OpenSSL API is, its actually not surprising ;)

BTW, Vladimir also made this useful little tool that would split up the build time into its components (that he also demonstrated on DConf): DBuildStat - that gave some interesting insights in an earlier attempt to reduce compile times.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 31, 2014

Member

PS: I wanted to integrate some simple profiling into DUB that could be enabled with a command line switch, but didn't get around to it yet. That would probably help a lot to quickly identify where the time is wasted.

Member

s-ludwig commented Jul 31, 2014

PS: I wanted to integrate some simple profiling into DUB that could be enabled with a command line switch, but didn't get around to it yet. That would probably help a lot to quickly identify where the time is wasted.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Jul 31, 2014

Contributor

Updated.

It would probably make sense to turn SSLContext into a class instead of an interface as some methods should apply to any SSL implementation.

So we could move the SSLContextKind, SSLPeerValidationCallback and SSLPeerValidationMode fields into an abstract class in vibe.stream.ssl. I don't think it's worth it though and it can always be done when there actually is another SSL implementation.

Contributor

MartinNowak commented Jul 31, 2014

Updated.

It would probably make sense to turn SSLContext into a class instead of an interface as some methods should apply to any SSL implementation.

So we could move the SSLContextKind, SSLPeerValidationCallback and SSLPeerValidationMode fields into an abstract class in vibe.stream.ssl. I don't think it's worth it though and it can always be done when there actually is another SSL implementation.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Jul 31, 2014

Contributor

To make another case for interfaces, it should now be possible to implement a SSL replacement or a different event driver in another library. Might be a good starting point to work on a native epoll or kqueue implementation.

Contributor

MartinNowak commented Jul 31, 2014

To make another case for interfaces, it should now be possible to implement a SSL replacement or a different event driver in another library. Might be a good starting point to work on a native epoll or kqueue implementation.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 31, 2014

Member

So we could move the SSLContextKind, SSLPeerValidationCallback and SSLPeerValidationMode fields into an abstract class in vibe.stream.ssl. I don't think it's worth it though and it can always be done when there actually is another SSL implementation.

I think so, too. They are used relatively rarely and are just two lines, so it doesn't relly buy much in terms of either performance or saving code.

To make another case for interfaces, it should now be possible to implement a SSL replacement or a different event driver in another library. Might be a good starting point to work on a native epoll or kqueue implementation.

Event divers are separate, they have always been full interfaces. But it's definitely good to have that possibility opened up for SSL streams, too. Now the only thing missing would be to have a registration facility for alternative implementations, so that createSSL(Stream/Context) creates the external implementation instead of the OpenSSL one.

Member

s-ludwig commented Jul 31, 2014

So we could move the SSLContextKind, SSLPeerValidationCallback and SSLPeerValidationMode fields into an abstract class in vibe.stream.ssl. I don't think it's worth it though and it can always be done when there actually is another SSL implementation.

I think so, too. They are used relatively rarely and are just two lines, so it doesn't relly buy much in terms of either performance or saving code.

To make another case for interfaces, it should now be possible to implement a SSL replacement or a different event driver in another library. Might be a good starting point to work on a native epoll or kqueue implementation.

Event divers are separate, they have always been full interfaces. But it's definitely good to have that possibility opened up for SSL streams, too. Now the only thing missing would be to have a registration facility for alternative implementations, so that createSSL(Stream/Context) creates the external implementation instead of the OpenSSL one.

s-ludwig added a commit that referenced this pull request Jul 31, 2014

Merge pull request #757 from MartinNowak/ssl_interfaces
Hide SSL implementation headers behind interfaces.

@s-ludwig s-ludwig merged commit 3a91b98 into vibe-d:master Jul 31, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@MartinNowak MartinNowak deleted the MartinNowak:ssl_interfaces branch Jul 31, 2014

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Jul 31, 2014

Contributor

Would be a good selling point to have GNUTLS and PolarSSL plugins.

version (VibeCustomSSL) {
    interface SSLFactory {
        SSLContext createSSLContext(SSLContextKind kind, SSLVersion ver = SSLVersion.any);
        SSLStream createSSLStream(Stream underlying, SSLContext ctx, SSLStreamState state, string peer_name = null, NetworkAddress peer_address = NetworkAddress.init);
    }
    void setSSLFactory(SSLFactory factory) { s_sslFactory = factory; }
}

SSLStream createSSLStream(Stream underlying, SSLContext ctx, SSLStreamState state, string peer_name = null, NetworkAddress peer_address = NetworkAddress.init)
{
    version (VibeNoSSL) assert(false, "No SSL support compiled in (VibeNoSSL)");
    else version (OpenSSL) {
        import vibe.stream.openssl;
        return new OpenSSLStream(DEPRECATION_HACK.init, underlying, cast(OpenSSLContext)ctx,
                                 state, peer_name, peer_address);
    } else version (VibeCustomSSL) {
        enforce(s_sslFactory, "No SSL Factory provided.");
        return s_sslFactory.createSSLStream(underlying, ctx, state, peer_name, peer_address);
    }
}

Or plain function pointers.

version (VibeCustomSSL)
    void setCreateSSLStream(typeof(&createSSLStream) func) { s_createSSLStream = func; }

SSLStream createSSLStream(Stream underlying, SSLContext ctx, SSLStreamState state, string peer_name = null, NetworkAddress peer_address = NetworkAddress.init)
{
    // ...
    } else version (VibeCustomSSL)
        return s_createSSLStream(/*Args*/);
}
Contributor

MartinNowak commented Jul 31, 2014

Would be a good selling point to have GNUTLS and PolarSSL plugins.

version (VibeCustomSSL) {
    interface SSLFactory {
        SSLContext createSSLContext(SSLContextKind kind, SSLVersion ver = SSLVersion.any);
        SSLStream createSSLStream(Stream underlying, SSLContext ctx, SSLStreamState state, string peer_name = null, NetworkAddress peer_address = NetworkAddress.init);
    }
    void setSSLFactory(SSLFactory factory) { s_sslFactory = factory; }
}

SSLStream createSSLStream(Stream underlying, SSLContext ctx, SSLStreamState state, string peer_name = null, NetworkAddress peer_address = NetworkAddress.init)
{
    version (VibeNoSSL) assert(false, "No SSL support compiled in (VibeNoSSL)");
    else version (OpenSSL) {
        import vibe.stream.openssl;
        return new OpenSSLStream(DEPRECATION_HACK.init, underlying, cast(OpenSSLContext)ctx,
                                 state, peer_name, peer_address);
    } else version (VibeCustomSSL) {
        enforce(s_sslFactory, "No SSL Factory provided.");
        return s_sslFactory.createSSLStream(underlying, ctx, state, peer_name, peer_address);
    }
}

Or plain function pointers.

version (VibeCustomSSL)
    void setCreateSSLStream(typeof(&createSSLStream) func) { s_createSSLStream = func; }

SSLStream createSSLStream(Stream underlying, SSLContext ctx, SSLStreamState state, string peer_name = null, NetworkAddress peer_address = NetworkAddress.init)
{
    // ...
    } else version (VibeCustomSSL)
        return s_createSSLStream(/*Args*/);
}
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 31, 2014

Member

Wait a minute... that DEPRECATION_HACK can actually be dropped now. It was only necessary for a non-breaking switch to interfaces. So VibeCustomSSL could also be avoided and the OpenSSLStream would just behave like any other implementation. Also, I see no reason against adding a createStream method to SSLContext, so that the registration would boil down to just a single SSLContext function():

void setSSLContextFactory(SSLContext function(SSLContextKind, SSLVersion) factory)
    { s_factory = factory }

void createSSLContext(SSLContextKind kind, SSLVersion ver)
    { return s_factory(kind, var); }

void createSSLStream(SSLStream underlying, SSLContext ctx, ...)
    { return ctx.createStream(underlying, ...);
Member

s-ludwig commented Jul 31, 2014

Wait a minute... that DEPRECATION_HACK can actually be dropped now. It was only necessary for a non-breaking switch to interfaces. So VibeCustomSSL could also be avoided and the OpenSSLStream would just behave like any other implementation. Also, I see no reason against adding a createStream method to SSLContext, so that the registration would boil down to just a single SSLContext function():

void setSSLContextFactory(SSLContext function(SSLContextKind, SSLVersion) factory)
    { s_factory = factory }

void createSSLContext(SSLContextKind kind, SSLVersion ver)
    { return s_factory(kind, var); }

void createSSLStream(SSLStream underlying, SSLContext ctx, ...)
    { return ctx.createStream(underlying, ...);
@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 31, 2014

Contributor

To make another case for interfaces, it should now be possible to implement a SSL replacement or a different event driver in another library. Might be a good starting point to work on a native epoll or kqueue implementation.

The sanest SSL stream would be Botan, I'll be using the algorithm factory for the TLS engine I'm working on but it does have a decent TLS engine already out of the box
https://github.com/randombit/botan/blob/net.randombit.botan/src/lib/tls/tls_server.h
https://github.com/randombit/botan/blob/net.randombit.botan/src/lib/tls/tls_client.h

Also, it has been shown to statically compile with an amalgamation script used in Titanium https://github.com/ellipticbit/titanium

It looks like it's a hot subject so I'm going to re-open my work on native events right now see if I can move it into a driver this week.

Contributor

etcimon commented Jul 31, 2014

To make another case for interfaces, it should now be possible to implement a SSL replacement or a different event driver in another library. Might be a good starting point to work on a native epoll or kqueue implementation.

The sanest SSL stream would be Botan, I'll be using the algorithm factory for the TLS engine I'm working on but it does have a decent TLS engine already out of the box
https://github.com/randombit/botan/blob/net.randombit.botan/src/lib/tls/tls_server.h
https://github.com/randombit/botan/blob/net.randombit.botan/src/lib/tls/tls_client.h

Also, it has been shown to statically compile with an amalgamation script used in Titanium https://github.com/ellipticbit/titanium

It looks like it's a hot subject so I'm going to re-open my work on native events right now see if I can move it into a driver this week.

@MartinNowak

This comment has been minimized.

Show comment
Hide comment
@MartinNowak

MartinNowak Jul 31, 2014

Contributor

It looks like it's a hot subject so I'm going to re-open my work on native events right now see if I can move it into a driver this week.

What are you working on, epoll, kqueue or something else?

Contributor

MartinNowak commented Jul 31, 2014

It looks like it's a hot subject so I'm going to re-open my work on native events right now see if I can move it into a driver this week.

What are you working on, epoll, kqueue or something else?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 31, 2014

Contributor

What are you working on, epoll, kqueue or something else?

I'm working on epoll and iocp at first. I put it on hold before finishing and making a pull request because I developed it too fast (10 days) and I wasn't sure if I would change my mind on certain things.

https://github.com/etcimon/vibe.d/tree/native-events/source/vibe/core/events

Contributor

etcimon commented Jul 31, 2014

What are you working on, epoll, kqueue or something else?

I'm working on epoll and iocp at first. I put it on hold before finishing and making a pull request because I developed it too fast (10 days) and I wasn't sure if I would change my mind on certain things.

https://github.com/etcimon/vibe.d/tree/native-events/source/vibe/core/events

etcimon added a commit to etcimon/vibe.d that referenced this pull request Aug 4, 2014

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