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

Use Content-Length Header in Form Uploads #1101

Merged
merged 3 commits into from May 12, 2015

Conversation

Projects
None yet
2 participants
@etcimon
Contributor

etcimon commented May 12, 2015

This optimization takes advantage of Content-Length if provided and avoids many calls to read/write, which gives me a 20% performance improvement.

Use Content-Length Header in Form Uploads
This optimization takes advantage of Content-Length if provided and avoids many calls to read/write, which gives me a 20% performance improvement.
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 12, 2015

Member

Instead of a manual buffered read loop, it should use the OutputStream.write overload that takes an InputStream and a length. At least in theory this opens up some optimization opportunities and reduces code size.

Member

s-ludwig commented May 12, 2015

Instead of a manual buffered read loop, it should use the OutputStream.write overload that takes an InputStream and a length. At least in theory this opens up some optimization opportunities and reduces code size.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon May 12, 2015

Contributor

Ah, yes of course. But right now I'm a little unsure about how to deal with any newline there could be before the next boundary.

Contributor

etcimon commented May 12, 2015

Ah, yes of course. But right now I'm a little unsure about how to deal with any newline there could be before the next boundary.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon May 12, 2015

Contributor

Ok the boundary includes a newline. One less thing to worry about

Contributor

etcimon commented May 12, 2015

Ok the boundary includes a newline. One less thing to worry about

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon May 12, 2015

Contributor

I'm currently writing an ecryption-enabled cookie jar in sqlite, which for the moment simply decrypts/decompresses/extracts a blob into a file, calls the file cookie jar, and puts it back in the db. I think a lot of session stuff could also use sqlite. To me it seems ideal for desktop applications that feature a web UI / portable server backend. Would you consider embedding the sqlite library with vibe.d or should this belong to another repository?

Contributor

etcimon commented May 12, 2015

I'm currently writing an ecryption-enabled cookie jar in sqlite, which for the moment simply decrypts/decompresses/extracts a blob into a file, calls the file cookie jar, and puts it back in the db. I think a lot of session stuff could also use sqlite. To me it seems ideal for desktop applications that feature a web UI / portable server backend. Would you consider embedding the sqlite library with vibe.d or should this belong to another repository?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 12, 2015

Member

I'd opt for a separate repository. I'll eventually drag out the MongoDB/Redis stuff, too, so it doesn't make much sense to include more database code now. Another thing is that sqlite doesn't really fit well with vibe.d, because it doesn't use AIO - at least not well enough to advertise it as part of the core distribution.

Member

s-ludwig commented May 12, 2015

I'd opt for a separate repository. I'll eventually drag out the MongoDB/Redis stuff, too, so it doesn't make much sense to include more database code now. Another thing is that sqlite doesn't really fit well with vibe.d, because it doesn't use AIO - at least not well enough to advertise it as part of the core distribution.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 12, 2015

Member

Regarding the read loop, I meant simply if (auto plen = ...) file.write(stream, (*plen).to!long);

Member

s-ludwig commented May 12, 2015

Regarding the read loop, I meant simply if (auto plen = ...) file.write(stream, (*plen).to!long);

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon May 12, 2015

Contributor

I'd opt for a separate repository. I'll eventually drag out the MongoDB/Redis stuff, too, so it doesn't make much sense to include more database code now. Another thing is that sqlite doesn't really fit well with vibe.d, because it doesn't use AIO - at least not well enough to advertise it as part of the core distribution

Sure, do you think it could appear as a use of "dub features" though? For example the way Botan was built, I kind of hacked my way through but the principle is to opt-in/opt-out of certain features and get a nice mix. The compile time and exec size seems to adjust very well to it too and it doesn't require building a separate repo for each little feature which would also involve having to find ways of detecting if the right "mix" is being used together.

Contributor

etcimon commented May 12, 2015

I'd opt for a separate repository. I'll eventually drag out the MongoDB/Redis stuff, too, so it doesn't make much sense to include more database code now. Another thing is that sqlite doesn't really fit well with vibe.d, because it doesn't use AIO - at least not well enough to advertise it as part of the core distribution

Sure, do you think it could appear as a use of "dub features" though? For example the way Botan was built, I kind of hacked my way through but the principle is to opt-in/opt-out of certain features and get a nice mix. The compile time and exec size seems to adjust very well to it too and it doesn't require building a separate repo for each little feature which would also involve having to find ways of detecting if the right "mix" is being used together.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 12, 2015

Member

I think we'll definitely need to have a "features" feature, but in this case a separate package seems like the logical choice. You can make dependencies optional, which often serves the same purpose as a "feature". I've already changed the code in the http2-botan branch to use version (Have_botan) to conditionally include the Botan based TLS implementation, so that it will hopefully work out without increasing the number of configurations.

Member

s-ludwig commented May 12, 2015

I think we'll definitely need to have a "features" feature, but in this case a separate package seems like the logical choice. You can make dependencies optional, which often serves the same purpose as a "feature". I've already changed the code in the http2-botan branch to use version (Have_botan) to conditionally include the Botan based TLS implementation, so that it will hopefully work out without increasing the number of configurations.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 12, 2015

Member

Oh, I wouldn't want to add it to vibe.d as a "feature" or optional dependency, though. It's simply a small library layered on top. At least that's how it should be in an ideal world.

Member

s-ludwig commented May 12, 2015

Oh, I wouldn't want to add it to vibe.d as a "feature" or optional dependency, though. It's simply a small library layered on top. At least that's how it should be in an ideal world.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon May 12, 2015

Contributor

Btw, I've also found some bugs when testing some more on my end (with a file server)

  • The outputStream can be null in which case the freeing shouldn't happen and the streams are still flushed (currently this fails the assertion): if (!outputStream) skip freeing
  • The outputStream doesn't necessarily exist when writing headers (the current code may trigger a segfault): should be StreamOutputRange(topStream)
  • The libasync driver has an assertion that slows everything down a lot when many writes are involved: should be removed
  • The libasync AsyncFile was sluggish for large files which is fixed in 0.7.4 (I tagged it just now).

I think these quick fixes belong in the http2-botan-cleanup so I'm pointing them out here

Contributor

etcimon commented May 12, 2015

Btw, I've also found some bugs when testing some more on my end (with a file server)

  • The outputStream can be null in which case the freeing shouldn't happen and the streams are still flushed (currently this fails the assertion): if (!outputStream) skip freeing
  • The outputStream doesn't necessarily exist when writing headers (the current code may trigger a segfault): should be StreamOutputRange(topStream)
  • The libasync driver has an assertion that slows everything down a lot when many writes are involved: should be removed
  • The libasync AsyncFile was sluggish for large files which is fixed in 0.7.4 (I tagged it just now).

I think these quick fixes belong in the http2-botan-cleanup so I'm pointing them out here

s-ludwig added a commit that referenced this pull request May 12, 2015

Merge pull request #1101 from etcimon/patch-7
Use Content-Length Header in Form Uploads

@s-ludwig s-ludwig merged commit 72599b9 into vibe-d:master May 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 12, 2015

Member

Okay, I'll paste these points to the http2 pull request, so that they won't get lost.

Member

s-ludwig commented May 12, 2015

Okay, I'll paste these points to the http2 pull request, so that they won't get lost.

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