Lock hash tables during augmented assignment

[d-torrance]

This is the beginnings of an attempt at fixing #3928.

Here's the idea: We'd like something like x.a += 1 to be atomic when x is a mutable hash table. So at the beginning of augmented assignment, we look to see if the left hand side is binary code with . or # as its binary operator, and then check if the left hand side of that is a hash table. If so, go ahead and lock it, and stuff the hash table in a variable so we know how to unlock it when we're done.

The big problem, however, is that we currently implement hash table mutexes with a rwlock, which isn't recursive. In particular, if we lock the mutex, we can't lock it again in the same thread or we deadlock. But we're definitely going to lock it at least one more time when we call lookup to get the value at the given key.

So in the current draft, M2 immediately aborts because it runs into this problem on startup.

A possible idea would be to switch from rwlock's to recursive mutexes for hash tables. But hash tables are so fundamental in M2 that I'd rather wait until after the release to try this to give us plenty of time to test any repercussions.

Cc: @mahrud

[jkyang92]

I feel like it would be cleaner to handle x#k op= y and x.k op= y separately since we're already matching on the structure of the left hand side. Then we can have a function that looks something like ModifyAndAssign(x,k,op,y) that does the actual operation on a hash table. This function can lock the table once, lookup the value and modify it. That way we aren't depending on the call to eval(x.lhs) to do the lookup.

The think I can't remember is if we could have a circumstance where "#" or "." is overridden with something weird.

[mahrud]

I don't think # or . can be overridden at top-level (yet!) but I consider this to be higher priority than allowing top-level to override. This is extremely troublesome for me right now so I'll take any solution that works!

[d-torrance]

. and # can't be overridden. I used to not like that, but now I kind of do. We have other operators (_, and I use @@ in the Python package for .) that we can use, keeping these two nice and low level for getting at the underlying list or hash table.

I like Jay's idea. I'll play around with it soon

[mahrud]

(I deeply dislike @@ btw, you should just move the relevant types to the interpreter and use . I think)

[jkyang92]

Also, I didn't mention it earlier, you have to make sure to evaluate the right hand side before locking on the table. This is a bit subtle about order of operations, but if the right hand side also uses the same hash table (which isn't crazy) then you would have the same recursive lock issue.

I think this makes x += y slightly different semantically from x = x + y. In particular if both the expressions x and y produce errors, which error do we get? Right now if x is a mutable hash table and 1 is not a key, then x#1 += error "test" produces error: key not found in hash table, but using the suggestion it would produce error: test

This is a compromise that I think is fine, but I want to point this out.

In theory a custom operator can make this even worse if it looks into the table somehow, but that shouldn't happen with sensible operators.

[mahrud]

@d-torrance I can't help until after the semester but any luck with this?

[d-torrance]

I haven't taken a stab at it yet, but hopefully soon!

[mahrud]

@jkyang92 in principle I agree with everything in your comment, but I struggle to see a clean way to implement it. Do you have a any suggestions?

[jkyang92]

@mahrud

Inside of the s:Symbol do (...) block inside of augmentedAssignmentFun, instead of first evaluating the left hand side, you can check if x.lhs is a reference into a hash table (or list) (binaryCode with a operator of DotS or SharpS), if it is, call a separate function that handles the case of hash tables, otherwise continue with the current code (not the code in the pull request). This check is essentially what maybeLock in this pull request is currently doing, but instead of conditionally locking, we just take a different code path.

In the hash table case, you can then:

1. Evaluate the right hand side
2. Lock the hash table
3. Get the value from the hash table
4. Apply the operator to the entry
5. Write the entry back to the hash table
6. Unlock the hash table

The only danger with this is if the operator has a user defined method, it could in theory relock the hash table in step 4.

You could also add a bit of code before step 1 to take a read lock on the hash table and check if the entry even exists before proceeding, I don't know that this expense is worth it.

[jkyang92]

Another approach that would have fewer worries about deadlock/relocking would be to "black hole" the entry in the hash table when we are modifying it. This entails putting a placeholder entry into the hash table that indicates "this entry is being modified". The black hole then should indicate which thread is currently modifying it, and have appropriate condition variables/mutexes such that other threads can wait on it. (If creating one per black hole seems expensive you can do one per thread). It's then an error (which can be sent back to the user) if you try to modify (but not read?) an entry which the current thread has black holed, but any other thread will just wait on the black hole. To avoid deadlock, you'd need special handling of waiting on black holed entries while having created black holed entries, but as long as you allow reads through the black hole, this is not a huge issue. The advantages is that this allows not holding the lock while doing the actual operation and only when actually writing the entry back to the hash table (you still have to take a write lock to add the black hole).

There are complicated questions about what sequencing of operations this corresponds to, and annoyingly, it requires a check for black holes at every write into the hash table.

[mahrud]

This is a funny compiler warning:

[45/59] Building C object Macaulay2/bin/CMakeFiles/M2-binary.dir/startup.c.o
/home/mahrud/Projects/M2/research/M2/BUILD/build/Macaulay2/bin/startup.c:196:58: warning: trigraph ‘??=’ ignored, use ‘-trigraphs’ to enable [-Wtrigraphs]
  196 |    "     setAttribute = (val,attr,x) -> ( Attributes#val ??= new MutableHashTable )#attr = x;\n"

Shouldn't the compiler know to ignore a hardcoded string?

ps: I actually don't think it's related to this PR but somehow this is the first I'm seeing this.

[jkyang92]

@mahrud It occurs very early in the processing, even before tokenization. so the compiler can't tell if it's in a string. If we want to suppress the warning, we can split the string into two (using the fact that neighboring string literals get concatenated). Also, I checked and trigraphs have been removed in C++17 and C23.

[mikestillman]

We are compiling with c++17, I thought?

[jkyang92]

It's because its a C file and we're certainly not compiling with C23.