esl / ice

Apache License 2.0
2 stars 4 forks source link

Review concurrent ets cache and improve it using Mnesia #57

Closed lucafavatella closed 10 years ago

lucafavatella commented 10 years ago

Review concurrent ets cache changes as per commits b12f0b7..f56649a, adding some documentation and proposing improvements.

Main changes:

This PR introduces a dependency on mnesia.

lucafavatella commented 10 years ago

Ed, The reason why you need all that code in ice_cache:add that diverges from the literature is explained in the comment in commit 11444ef of this PR. I need to look for transaction/lock-like operations in ets, otherwise we need mnesia in-memory table for transactions. I will continue looking at this in my review-cache-dev branch.

lucafavatella commented 10 years ago

Ed, I propose in this PR a draft "improvement" for the concurrent cache that fixes the lack of atomitity in the beta.find() operation using mnesia instead of ets. The PR is not in a mergeable state as the current eunit test failures shall be investigated. They look all related to the handling of multiple dimensions in the cache (e.g. failing test in tcache_tests) but I have not investigated them further yet.

lucafavatella commented 10 years ago

Re getting the existing value in the table when ets:insert_new returns false, I think this is a generic issue/feature in the ets interface: destructive operations do not give visibility of what was in the table at the moment of destructing data. Some ets functions affected by this "issue":

I have not investigated this further, but I think this is related to make ets behave as a tuple space https://en.wikipedia.org/wiki/Tuple_space I think this was mentioned by Alejandro Ramallo at the Erlang User Group some time ago:

So maybe an RFC for ets to erlang-patches could gain some traction.

lucafavatella commented 10 years ago

Ed, Tests are fixed, it was a problem with missing dimensions between ice_cache and ice_dtree. The code could be polished but I would like your feedback before doing that (maybe you prefer to new ets bif rather than introducing mnesia).

lucafavatella commented 10 years ago

Ed, I think this pull request is ready for a review. Please let me know you feedback.

lucafavatella commented 10 years ago

I coded another (I believe) improvement in ice_dtree:lookup/2 function. It is in my review-cache-dev branch, commit e62d7e766e670972fa0cd4b8232ceac747d7078e. I think it makes more sense, it passes make test but I have not written a test for it yet.

et4te commented 10 years ago

Luca I am about to commit a huge set of changes to renaming, could you rebase after I do that and then we will merge.

lucafavatella commented 10 years ago

Sure. I do not see the renaming changes in master yet.

et4te commented 10 years ago

Hi please rebase.

lucafavatella commented 10 years ago

Ed, I rebased and added another commit where I remove the third element of the internal node of the cache as discussed via emai.