Add persistent_term:put_new

[Benjamin-Philip]

This PR adds the persistent_term:put_new to conditionally add a key-value only if the key is new.

Closes #9681.

/cc @jhogberg.

[github-actions]

CT Test Results

    3 files    142 suites   50m 17s ⏱️
1 651 tests 1 593 ✅ 57 💤 1 ❌
2 374 runs  2 296 ✅ 77 💤 1 ❌

For more details on these failures, see this check.

Results for commit abb29a7.

♻️ 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

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[josevalim]

Instead of adding put_new, would it be more useful to add a has_key function that could be used in more cases? I recognize put_new has the benefit of being atomic but I assume that the value you need to store is likely expensive to compute anyway, so you want to check if the key exists before you do any work.

[Benjamin-Philip]

I think a has_key would be a good addition, but not as a replacement to put_new. Like you said put_new has the benefit of being atomic which enables one to "correctly" (in a formal sense) update parallely out of the box. See the linked issue for my use case.

You can define a rudimentary has_key already by setting Default in get/2 to undefined or some other more specific atom. Nevertheless, has_key would be faster and less error prone.

TLDR; I think we should add both functions.

[Benjamin-Philip]

@jhogberg, I think this PR is review ready now. Sorry about the delay - I was busy the past 2 weeks...

[Benjamin-Philip]

Rebased on master to fix merge conflicts.

[Benjamin-Philip]

@jhogberg, Any updates on this? Do you think you can squeeze it in OTP 28? Also, do you think has_key should be added (in another PR, of course) as @josevalim suggested?

[jhogberg]

@jhogberg, Any updates on this? Do you think you can squeeze it in OTP 28?

As mentioned in the issue, the window for 28 was closed before you opened it. It can be included in 28.1 at the very earliest.

The refactor I mentioned is coming along well. It's feature-complete and offers far better add/update/erase performance (so long as literal GC isn't triggered), but read performance on dynamic keys has taken a hit that is proving a bit annoying to get rid of (statically known keys are effectively free however). I'm confident it'll work out however, and am aiming for OTP 29.

Also, do you think has_key should be added (in another PR, of course) as @josevalim suggested?

I can't see why not, I'll add it to my refactored version.

[Benjamin-Philip]

It can be included in 28.1 at the very earliest.

When do you think 28.1 will be released?

[jhogberg]

When do you think 28.1 will be released?

Sorry for the late reply -- I missed the notification. 28.1 will be released early this autumn (September-ish).

[Benjamin-Philip]

@jhogberg, I've rebased to master. I'm hoping you can accept patches for OTP 28.1 now?

PS: If your refactor is public, could you share a link? I'm curious what are the changes you plan to make.

[jhogberg]

@jhogberg, I've rebased to master. I'm hoping you can accept patches for OTP 28.1 now?

28.1 would be maint, it should be relatively easy to rebase to that though since master hasn't moved in this area. :-)

PS: If your refactor is public, could you share a link? I'm curious what are the changes you plan to make.

I've paused work on it because more important things came up. The idea was to use concurrent tries to improve update performance for trivial values, sacrificing a bit of read performance but hopefully regaining it through inline caching. Inline caching of static keys resulted in great performance (marginally more expensive than reading a list head...), but I couldn't make dynamic keys fast enough.

I left it just before introducing namespacing, replacing the implicit persistent_term:put({?MODULE, Key}, Value) with an explicit persistent_term:put(?MODULE, Key, Value) where the user could define how they wanted their namespace to work -- slow writes but fast reads through the old hashtable method, or slightly slower reads but fast writes through concurrent tries.

I've pushed what I've got to https://github.com/jhogberg/otp/tree/john/erts/ctrie-experiment, adding put_new/2 would be as simple as adding a boolean argument to persistent_term_put to BIF_ERROR(c_p, BADARG) instead of calling pt_ctrie_replace.

[CLAassistant]

All committers have signed the CLA.

[Benjamin-Philip]

28.1 would be maint, it should be relatively easy to rebase to that though since master hasn't moved in this area. :-)

I've rebased to maint and changed the PR base from master to maint as well.

I've paused work on it because more important things came up. The idea was to use concurrent tries to improve update performance for trivial values, sacrificing a bit of read performance but hopefully regaining it through inline caching. Inline caching of static keys resulted in great performance (marginally more expensive than reading a list head...), but I couldn't make dynamic keys fast enough.

I left it just before introducing namespacing, replacing the implicit persistent_term:put({?MODULE, Key}, Value) with an explicit persistent_term:put(?MODULE, Key, Value) where the user could define how they wanted their namespace to work -- slow writes but fast reads through the old hashtable method, or slightly slower reads but fast writes through concurrent tries.

I think namespacing is a great idea - you reduce module, key pairs into a trivial atom. If you're creating a new hashtable/ctrie for each namespace you also get some additional isolation between namespaces in terms of failures and maybe even GC.

Regarding static and dynamic keys, do you mean keys known at compile time? Maybe we could cache all trivial keys (i.e. a key that is an atom)? This may not scale well with persistent terms as they are now, but if caching like this for each namespace was configured intentionally on a case by case basis it may work quite well.

[jhogberg]

I've rebased to maint and changed the PR base from master to maint as well.

It looks like you dragged some unrelated commits into it.

I think namespacing is a great idea - you reduce module, key pairs into a trivial atom. If you're creating a new hashtable/ctrie for each namespace you also get some additional isolation between namespaces in terms of failures and maybe even GC.

It doesn't help GC at all, but it does help lookups and update speed as unrelated things won't need to be considered.

Regarding static and dynamic keys, do you mean keys known at compile time? Maybe we could cache all trivial keys (i.e. a key that is an atom)? This may not scale well with persistent terms as they are now, but if caching like this for each namespace was configured intentionally on a case by case basis it may work quite well.

Any key known at load-time (regardless of complexity) was cached at load-time in my prototype, and scaled perfectly without namespaces, so it's something we'll want to do regardless. The barrier required to check if the value was stale (and update if so) was effectively free in my tests as long as it wasn't triggered.

Dynamic keys are the tricky bit, where namespacing mainly help by splitting up different use cases; the concurrent tries were miles ahead of the old hash table on updates, and only slightly behind on lookups. If your use-case requires quick updates, you might accept the small loss in lookup speed from using concurrent tries, and otherwise you could still use the old method.

[Benjamin-Philip]

It looks like you dragged some unrelated commits into it.

I rebased and forced pushed before updating the PR's base from master to maint. So GitHub dragged commits from maint that weren't on master.

I'm going to make a new branch and cherry-pick my commits on to it and update the PR source branch.

Edit: All fixed. Forced pushed from this new branch and renamed it to the old branch's name.

[Benjamin-Philip]

I've managed to unify trapping to a put_common_trap. However, I'm still facing some issues.

Edit: @jhogberg, do you have any suggestions?

[Benjamin-Philip]

@jhogberg , I've managed to push a working trap that passes all the tests within persistent_term_SUITE. However, I am facing 2 seemingly unrelated issues:

1. CHECK_TERM(*c_p->arg_reg[i])) in beam/beam_common.c fails on init and stop (see previous CI)
2. I have random segfaults on make emulator_test (see current CI).

Running cerl to check 1 in gdb and inspecting c_p, I can confirm that it's during the trap. From what I can tell, one of the trap arguments have not been set correctly. However, I don't understand why I couldn't reproduce this in the unit tests.

Any idea what gives?

[jhogberg]

The issues are probably related. Which of the arguments is it?

In the CI run it crashed during persistent_term_SUITE:get_all_race/1, suggesting that the problem is not specific to the new-key part of put_common, at least.

[Benjamin-Philip]

@jhogberg, I pushed a working solution few days ago. Not sure why the 32-bit and cross-compile builds failed though. Everything else seems to be in order.

[jhogberg]

Thanks, did you find the cause of the crashes? :)

[jhogberg]

Did something happen with the formatting here?

[jhogberg]

Please remove this and the other unrelated whitespace change further below, it makes git blame less effective.

[Benjamin-Philip]

I never did find the cause, except that it was while trapping. The latest implementation does not crash.

[Benjamin-Philip]

About the formatting, I reformatted the functions I was working on with Emacs' c-fill-paragraph. I'll revert the changes. Do you have a configuration for better formatting?

[jhogberg]

About the formatting, I reformatted the functions I was working on with Emacs' c-fill-paragraph. I'll revert the changes. Do you have a configuration for better formatting?

I believe the problem is that, at least in persistent_term_erase_1, there's a missing ; just before the formatting becomes weird.

I never did find the cause, except that it was while trapping. The latest implementation does not crash.

Alright, we'll see if it crashes in the nightly runs. :-)

[Benjamin-Philip]

Fixed the formatting. You were right about the semicolon: https://www.gnu.org/software/emacs/manual/html_node/ccmode/Macros-with-_003b.html

I suggested that OTP moves towards something like clang-format for more consistent and reproducible formatting in an issue a few months ago. Maybe we should revisit that again. If I remember correctly, the main reason for rejecting it was that it would introduce a lot of merge conflicts.

[Benjamin-Philip]

@jhogberg Any further changes? Is maint still the right branch to merge into?

[jhogberg]

Not at the moment, maint is still the right branch. It’s been tested briefly without anything fishy showing up, and it’ll get some more testing after 28.3 is released this Friday. 🙂