Skip to content

add ets take_first/1 and take_last/1 functions #5896

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nyczol
Copy link

@nyczol nyczol commented Apr 16, 2022

Add ets:take_last/1 and ets:take_first/1 func similar to ets:take/2, but return firts/last objects from ets.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 16, 2022

CT Test Results

       4 files     197 suites   1h 7m 28s ⏱️
2 914 tests 2 825 ✔️   89 💤 0
3 505 runs  3 395 ✔️ 110 💤 0

Results for commit 17203dc.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@garazdawi garazdawi added the team:VM Assigned to OTP team VM label Apr 19, 2022
Comment on lines 1205 to 1212
cret = tb->common.meth->db_first(BIF_P, tb, &first);

if (cret != DB_ERROR_NONE) {
db_unlock(tb, LCK_WRITE_REC);
BIF_ERROR(BIF_P, BADARG);
}

cret = tb->common.meth->db_take(BIF_P, tb, first, &ret);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No decision has been made yet to accept this API addition, but I just noticed that

you don't treat '$end_of_table' (first==am_EOT). It just happens to work as long as there is no such key.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize now that if first returns am_EOT then no keys at all exist. But it could actually be a race where someone else inserts key '$end_of_table' and it would make the code more clear to treat am_EOT.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nyczol To make a decision we would want a real life use case and what problem the new API addition solves.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that '$end_of_table' atom can't be used as a key (https://www.erlang.org/doc/man/ets.html).
Functions take_first/1 and take_last/1 can be used to build very efficient FIFO queues using ets.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking agains am_EOT has been added

@sverker
Copy link
Contributor

sverker commented Apr 20, 2022

I don't think we want to rush this into OTP 25. Especially as I discovered this implementation is not correct for table option write_concurrency. Two concurrent calls may get the same key from db_first and then race each other calling db_take where only one can succeed.

@CLAassistant
Copy link

CLAassistant commented Apr 22, 2023

CLA assistant check
All committers have signed the CLA.

@nyczol
Copy link
Author

nyczol commented Apr 22, 2023

I've just fix concurrency issue.
Now table is locked in LCK_WRITE except LCK_WRITE_REC.

@sverker
Copy link
Contributor

sverker commented Apr 25, 2023

It was fast and now it's correct. I think we should aim for both.

Keep LCK_WRITE_REC and implement new meth->take_first() functions for hash, tree and catree where we seize the lock once and release it when done.

@sverker sverker added the waiting waiting for changes/input from author label Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM waiting waiting for changes/input from author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants