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

Libasync ManualEvent in FileStream - fixes #1225 #1227

Merged
merged 2 commits into from Oct 30, 2015

Conversation

Projects
None yet
2 participants
@etcimon
Contributor

etcimon commented Aug 16, 2015

This should avoid the resumeTask issue, and with timeout for an added bonus

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Aug 16, 2015

Member

Isn't yieldAndResumeTask the correct fix here? Using a ManualEvent sounds like redundant overhead, since everything is in a single thread anyway (right?). What would happen if the timeout gets hit?

Member

s-ludwig commented Aug 16, 2015

Isn't yieldAndResumeTask the correct fix here? Using a ManualEvent sounds like redundant overhead, since everything is in a single thread anyway (right?). What would happen if the timeout gets hit?

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 16, 2015

Contributor

I recently made the read/write happen in the current thread when the buffer is < 65kb (through a libasync change). The mutex is not worth it for small reads. The task still registers itself in m_task at the scope start, which should be only if it waits, but I guess a boolean m_waiting would be required too or I'd have to move the acquireReader. I think I had a pull request for that previously.

So I guess this was a quick fix that worked in every situation and I didn't have time to break it down to a better m_reader and m_writer that get defined when the task starts waiting. I'm at a point where some unpolished modifications are permanent because it works for me and I need to finish a project without breaking stuff needlessly right now.

What would happen if the timeout gets hit?

The intended result is that an exception is thrown and internal server error gets returned. The timeout would have to be proportional to the read size, but it reads small chunks 99.9% of the time. I'd rather see a TimeoutException for better error reports though.

Contributor

etcimon commented Aug 16, 2015

I recently made the read/write happen in the current thread when the buffer is < 65kb (through a libasync change). The mutex is not worth it for small reads. The task still registers itself in m_task at the scope start, which should be only if it waits, but I guess a boolean m_waiting would be required too or I'd have to move the acquireReader. I think I had a pull request for that previously.

So I guess this was a quick fix that worked in every situation and I didn't have time to break it down to a better m_reader and m_writer that get defined when the task starts waiting. I'm at a point where some unpolished modifications are permanent because it works for me and I need to finish a project without breaking stuff needlessly right now.

What would happen if the timeout gets hit?

The intended result is that an exception is thrown and internal server error gets returned. The timeout would have to be proportional to the read size, but it reads small chunks 99.9% of the time. I'd rather see a TimeoutException for better error reports though.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 16, 2015

Contributor

Hm I just remembered why I used a ManualEvent. The handler in the LibasyncFileStream gets called by a foreign thread in the thread pool if size>65kb, so the task would be resumed from a foreign thread.

Contributor

etcimon commented Aug 16, 2015

Hm I just remembered why I used a ManualEvent. The handler in the LibasyncFileStream gets called by a foreign thread in the thread pool if size>65kb, so the task would be resumed from a foreign thread.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Aug 16, 2015

Contributor

, so the task would be resumed from a foreign thread.

I just checked and this was a wrong assumption . I had forgotten using AsyncSignal behind the scenes, so this would work correctly without ManualEvent

Contributor

etcimon commented Aug 16, 2015

, so the task would be resumed from a foreign thread.

I just checked and this was a wrong assumption . I had forgotten using AsyncSignal behind the scenes, so this would work correctly without ManualEvent

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 4, 2015

Contributor

It's already been almost 2 months. Been busy, I'll fix this tomorrow

Contributor

etcimon commented Oct 4, 2015

It's already been almost 2 months. Been busy, I'll fix this tomorrow

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 29, 2015

Member

Still got build failures on < 2.069 (2.069 failure unrelated).

Member

s-ludwig commented Oct 29, 2015

Still got build failures on < 2.069 (2.069 failure unrelated).

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 29, 2015

Contributor

Would you consider adding the ConnectionClosedException? They happen quite frequently and litter the logs if they're not caught separately

Contributor

etcimon commented Oct 29, 2015

Would you consider adding the ConnectionClosedException? They happen quite frequently and litter the logs if they're not caught separately

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 29, 2015

Member

Yes, sounds like a good addition. However, I'd probably postpone that to after the release, because all drivers should be adjusted accordingly.

Member

s-ludwig commented Oct 29, 2015

Yes, sounds like a good addition. However, I'd probably postpone that to after the release, because all drivers should be adjusted accordingly.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 29, 2015

Contributor

Out of context, but do you intent to add the new fast (json) https://github.com/mleise/fast simd optimizations to your std.json library?

Contributor

etcimon commented Oct 29, 2015

Out of context, but do you intent to add the new fast (json) https://github.com/mleise/fast simd optimizations to your std.json library?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Oct 29, 2015

Member

Not directly, but in the long term that would definitely make sense and there is nothing that would preclude it. However, the library is GPL, so no code can be directly taken form there.

Member

s-ludwig commented Oct 29, 2015

Not directly, but in the long term that would definitely make sense and there is nothing that would preclude it. However, the library is GPL, so no code can be directly taken form there.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Oct 29, 2015

Contributor

I don't think json parsing would be my bottleneck (or anyone's) anyways in a web server, so it's not worth concentrating efforts more than necessary. I think in the real world the only advantage is from having custom inlinine or parsing strings faster with vpcmpistri.

Currently my bottleneck is in the botan.math module for public key crypto (because of bigint math and the lack of inlinine) and in the pthread_mutex_lock/unlock which could be solved with version(TLSGC) but I need to experiment more

Contributor

etcimon commented Oct 29, 2015

I don't think json parsing would be my bottleneck (or anyone's) anyways in a web server, so it's not worth concentrating efforts more than necessary. I think in the real world the only advantage is from having custom inlinine or parsing strings faster with vpcmpistri.

Currently my bottleneck is in the botan.math module for public key crypto (because of bigint math and the lack of inlinine) and in the pthread_mutex_lock/unlock which could be solved with version(TLSGC) but I need to experiment more

@s-ludwig s-ludwig removed the needs input label Oct 30, 2015

s-ludwig added a commit that referenced this pull request Oct 30, 2015

Merge pull request #1227 from etcimon/patch-19
Libasync ManualEvent in FileStream - fixes #1225

@s-ludwig s-ludwig merged commit 3b0c3b7 into vibe-d:master Oct 30, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment