Dictionary#*(): remove bool overrideFrozen if unused

[Al2Klimov]

No description provided.

[Al2Klimov]

@julianbrost In addition to #9657 I'd like to do the full set of things as in #9627. However for this I have to remove overrideFrozen from these as well, in reverse order:

• Dictionary::Set
  • Dictionary::SetFieldByName
    • Object::SetFieldByName
      • VMOps::SetField
        • SetExpression::DoEvaluate
          • SetExpression::SetOverrideFrozen()
            • -D
        • IndexerExpression::GetReference
          • IndexerExpression::SetOverrideFrozen()
            • -D

Removing it from -D would mean I couldn't override (pre-)frozen things even with -D on the command line. Would this be reasonable?

[Al2Klimov]

See also 1a8692d#diff-5fa8a2db790ac74452eea918bf6185488bdb3fafc6c53b9e3aa1b53d496d7039

[julianbrost]

The command line flag --define is the only remaining use of overrideFrozen? With #9627, writes to frozen namespaces aren't possible at this point any more (see also the small incident with #9648). In general, are there even any dictionaries reachable from --define? If there are some, and these should be written to, they should just be frozen later.

[Al2Klimov]

Forget this. Proceeding would only be useful if one could freeze ConfigObject#vars. And we can't. Or would you accept the following even more radical change?

API modifications of ConfigObject#vars have to internally re-assemble the whole vars dict as it gets frozen.

[Al2Klimov]

TODO

• Use a shared_mutex for read Dictionary operations #9657

[julianbrost]

API modifications of ConfigObject#vars have to internally re-assemble the whole vars dict as it gets frozen.

So basically copy on write? I've been considering that for namespaces, main issue with that is how do you know when it's safe to delete the old version? If you can't guarantee that there are no concurrent operations, you'd have to ref-count the individual versions. I don't know how this would compare exactly to a rwlock, but it isn't free.

[Al2Klimov]

I would -if you explicitly agree- continue with the following.

• On API-modification of ConfigObject#vars -which should happen far less often that reads- re-assemble and replace vars ignoring any race conditions or -even better- locking the object
• Dictionary#*(): remove bool overrideFrozen if unused #9658 (comment) (you're right, as we've also frozen namespaces, we can also do this)
• Freeze ConfigObject#vars after individual commit

[julianbrost]

ignoring any race conditions

Which would probably result in use after free bugs.

or -even better- locking the object

Which you would also have to do for read operations. Then you can just lock the dictionary instead.

[Al2Klimov]

Just to be clear:

• Are you concerned about which-update-wins in addition to SEGV?
• If I can avoid that kind of bug, do I have green light for Dictionary#*(): remove bool overrideFrozen if unused #9658 (comment) ?

[julianbrost]

Are you concerned about which-update-wins in addition to SEGV?

Primarily, I'm concerned about introducing undefined behavior, mainly in situations where you might delete the old data while there's still some other thread accessing it.

In regarding two writes: well, they have to be sequenced. If one thread adds key A and another thread adds key B, there must not be a situation where the resulting dict only contains one of these keys. If two threads update the same key, the resulting dict may contain either value, unless the threads were synchronized externally.

If I can avoid that kind of bug, do I have green light for Dictionary#*(): remove unused bool overrideFrozen #9658 (comment) ?

Please elaborate what you're planning to do. So far I don't see what you want to change so that it results in an improvement and doesn't introduce a bug.

[Al2Klimov]

#9627 has already shown us that not locking at all -if frozen- is faster than read-locking. I want this for ConfigObject#vars -which are frequently accessed by child apply rules- immediately after individual commit. Therefore I have to get rid of ALL overrideFrozen -you said no problem for -D- and to add transparent CoW to ConfigObject#vars.

Let's -as 🇷🇺ns say- don't divide the fur of a not killed bear or -in our case- don't worry about not yet introduced bugs, but have a look at the implementation details of the stuff mentioned above once done. OK?

[julianbrost]

and to add transparent CoW to ConfigObject#vars.

Either you have an ingenious idea or this might be harder than you think.

As you said ConfigObject#vars instead of Dictionary, does this mean that you want to implement CoW outside of the Dictionary class?

[Al2Klimov]

This is exactly my plan.

[julianbrost]

Which means you have to track down all references that may be used to modify it. Also, nested dicts could be tricky. Could easily result in a huge changeset. But go ahead and give it a try.

[Al2Klimov]

Sigh. One does not simply un-freeze things and nobody supports atomic smart pointers w/o mutexes. #9657 must be enough.

[julianbrost]

What's the goal here now? Doesn't this remove overrideFrozen from dicts which is still reachable from --define (#9658 (comment))?

In general, are there even any dictionaries reachable from --define? If there are some, and these should be written to, they should just be frozen later.

So what about this part?

#9657 must be enough.

Sounds like this was intended to build something more on this PR. Do you plan to build something else on this (as you asked if you can use this as a base for another PR)?

[Al2Klimov]

Now this is only a cleanup of unused-s. Look at the individual commits. They reduce the set of function signatures like {(whatever...), (whatever..., bool)} to just {(whatever...)} assuming the bool is always false with cleanup implications in the impl. details. Now -as there were no users of (whatever..., bool)- the compiler doesn't complain.

[julianbrost]

The PR title is ambiguous: I've understood it as "overideFrozen is not used in Dictionary, remove it" whereas it is meant to say "some instances of overrideFrozen in Dictionary are unused, those are removed, others remain".

[Al2Klimov]

Better?