search

treeform / puppy

Puppy fetches via HTTP and HTTPS
MIT License
184 stars 27 forks source link

Loony as an alternative implementation for RequestPool queue #51

Closed shayanhabibi closed 1 year ago

shayanhabibi commented 2 years ago

While the work being done by your request pools is primarily IO blocking, loony does not employ the use of locks and has a deep testing for memory coherency.

As you can see, it should greatly simplify the code of your request pools.

My naive tests with your test_requestpools did not yield any significant difference. I am well aware this is most likely due to the test being IO bound. No surprises there.

treeform commented 2 years ago

It appears that you have a loony version missmatch on Ubuntu:

/home/runner/.nimble/pkgs/loony-0.1.10/loony/ward.nim(203, 5) Error: undeclared identifier: 'wakeAll'
shayanhabibi commented 2 years ago

It appears that you have a loony version missmatch on Ubuntu:

/home/runner/.nimble/pkgs/loony-0.1.10/loony/ward.nim(203, 5) Error: undeclared identifier: 'wakeAll'

The issue should be corrected now

guzba commented 2 years ago

Thanks for preparing this PR. I am aware of the work being done on Loony and think it looks really cool. We'll take a look at this PR asap.

shayanhabibi commented 2 years ago

I'll look into what primitive is available for macosx as a futex and implement it. They are less likely to result in dead runtimes and are preferred over conditions and locks.

Edit: From what I can see there isnt an alternative on Mac OS. Will have to use a condition and lock to achieve this. I'll commit to this branch when I have implemented this for Loony futexes.

shayanhabibi commented 2 years ago

I'll have to close this PR for now and revisit it once I have determined a suitable method for emulating futex behaviour with Loony for Darwin based Operating Systems. It seems like it will need more fiddling than I originally anticipated to keep the API and code easy to understand and work with.

Thanks for the interest!

treeform commented 2 years ago

I look forward to see what you will come up with.

shayanhabibi commented 2 years ago

In response to the discussion you were having on the nim repo, concurrency with ref objects will always have the possibility of undefined behaviour unless you change the behaviour of system/arc.

With extensive testing it has been found that rapid transactions between threads consuming a ref count with another thread allocating ref objects will eventually result in a SIGSEGV when the allocating thread tries to allocate on dirty memory. It is possible to use memory fences to fix this on the user side, but it's not really worth it. System/arc needs to have its operations to be atomic if it is to avoid this undefined behaviour.

This is unlikely to occur when threads are performing work with the ref objects, however it is something to be aware of. Loony can only synchronise volatile memory inbetween pushing and popping. The decrefs that occur outside of that are not synchronised.

Containing the ref objects destruction and subsequent allocation within lock acquire/release would be fine. Manual handling of memory and ref counts are the most per formative and safe solution for this behaviour until arc is made thread friendly.

This will be fixed in nimskull but it's up to someone else to apply the fixes to nim.

shayanhabibi commented 2 years ago

ulock primitives on darwin like OS with appropriate vers compatibility (which includes upwards of 95% of mac operating systems) has been implemented for loony.

You are free to consider the implementation of Loony with puppy if it is of interest.

treeform commented 2 years ago

Lets see if it passes the mac tests! Exciting. Does it also work with arm M1 macs?

treeform commented 2 years ago

I think you have some extra stuff in there now? Like chaining readme.

guzba commented 2 years ago

I have been working in other repos so didn't notice this for a bit but I have a tab open to it now to remind me. If the PR can get to a merge-able point lets take another look, this was interesting.

shayanhabibi commented 2 years ago

Sorry I was forking puppy to an org handle so I could drill only into the windows api and operations (for a pharmacy app that is in a windows environment). Got messy with the git operations.

Since you like the idea of it, I will rebase it and ping for reviews when everything works as expected.

guzba commented 1 year ago

Hey, I wanted to post a late update here. We have concluded that the request pooling approach we were exploring isn't the one we want to go with. We removed it here https://github.com/treeform/puppy/pull/78 since we do not want to be responsible for maintaining it long-term without having any need of it. I am closing this PR since it's not the path we're taking.

If you were using this yourself or know others who are, having a repo for pooled HTTP requests seems like a reasonable idea. It's just not something we want to take on.

I personally would rather use the async request support already offered by the same system APIs that Puppy wraps instead of requiring --threads:on and worrying about threading stuff for maybe not any good reason when system APIs already handle it and are going to be used either way.

Thanks for the PR though and sorry it didn't end up being an area we ended up focusing on. It's hard to know in advance what'll really work out so we just have to explore and learn.