Finish stable split
ClosedPublic

Authored by dfeuer on Aug 21 2018, 10:50 PM.

Details

Summary

Long ago, the stable name table and stable pointer tables were one.
Now, they are separate, and have significantly different
implementations. I believe the time has come to finish the split
that began in Trac #7674.

  • Divide rts/Stable into rts/StableName and rts/StablePtr.
  • Give each table its own mutex.
  • Add FFI functions hs_lock_stable_ptr_table and hs_unlock_stable_ptr_table and document them. These are intended to replace the previously undocumented hs_lock_stable_tables and hs_lock_stable_tables, which are now documented as deprecated synonyms.
  • Make eqStableName# use pointer equality instead of unnecessarily comparing stable name table indices.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dfeuer created this revision.Aug 21 2018, 10:50 PM

I should also split includes/rts/Stable.h. That's pretty trivial.

I haven't done a full review but this seems quite sensible to me. It always seemed like these were unnecessarily intertwined.

@bgamari, what do you know about the FFI bits I tentatively removed? Should I put them back (locking and unlocking both tables)? Should I (alternatively or in addition) add the ability to lock and unlock the tables separately?

simonmar added inline comments.Aug 23 2018, 12:35 PM
rts/HsFFI.c
31–41

These only make sense in conjunction with hs_free_stable_ptr_unsafe. I think we should keep them - it seems like a reasonable thing to want to free a ton of stable ptrs at the same time, and save the locking overhead. It wouldn't surprise me if this was undocumented though.

There might be code using these in the wild. My memory is a bit hazy but I think we added them at the request of folks at Standard Chartered.

dfeuer added inline comments.Aug 23 2018, 7:34 PM
rts/HsFFI.c
31–41

So they really should only luck and unlock the stable *pointer* table, right? Should I write new functions whose names reflect that, and make these deprecated synonyms? (How does one deprecate things in C?) Also, where should the documentation go? It'll need to reflect that it's illegal to call into Haskell code between locking and unlocking the table.

dfeuer updated this revision to Diff 17790.Aug 24 2018, 2:29 PM
  • Clean up
dfeuer marked an inline comment as done.Aug 24 2018, 2:44 PM

I put the FFI functions back, with the same names, but only locking and unlocking the stable pointer table, since there's no other FFI access to the stable name table. I also split the last little header file that held these tables together.

dfeuer updated this revision to Diff 17832.Aug 27 2018, 2:36 PM

Fix up the comparison business

dfeuer updated this revision to Diff 17835.Aug 27 2018, 3:38 PM
  • Document unsafe stable pointer operations
dfeuer added inline comments.Aug 27 2018, 3:46 PM
rts/StableName.c
40–41

Do we actually have to lock the stable name table for GC, or just the stable pointer table? The stable pointer table is accessible through the FFI, but the stable name table is not.

dfeuer updated this revision to Diff 17837.Aug 27 2018, 7:35 PM
  • Document unsafe stable pointer operations
Harbormaster failed remote builds in B22481: Diff 17834!
Harbormaster failed remote builds in B22482: Diff 17835!
simonmar added inline comments.Aug 28 2018, 8:30 AM
rts/HsFFI.c
31–41
  1. Yes
  2. Yes
  3. There's no way, just add comments and don't document the deprecated things
  4. In the User's Guide, we have a section on GHC-specific extensions to the FFI

Thanks for cleaning this up :)

@simonmar, I'll get those FFI names sorted. My remaining question is whether we need to lock the stable name table for GC, or only the stable pointer table. The code doesn't explain why the garbage collector locked the stable tables. Is that because there could be C code hoping to free stable pointers while GC is in progress, or is there another reason?

Also, it appears that GCC, clang, and at least some other C compilers support a __deprecated__ attribute that we should be able to use. Should I just boldly use it, or should I try to use CPP somehow to check if the compiler supports it?

dfeuer updated this revision to Diff 17849.Aug 28 2018, 4:04 PM
  • More cleanup
  • Don't lock and unlock the stable name table in GC. The Haskell world should be stopped, and the C world has no access. This means the only place we have an extra lock to acquire and release is in forkProcess, which has to hold all the locks.
  • Add better names for the FFI stable pointer table locking and unlocking operations and deprecate the old ones.

@simonmar, based on @bgamari's understanding and my own, I've removed the code to lock and unlock the stable name table during GC. If that's wrong, let me know. I've also updated the FFI names and added deprecation attributes for gcc and clang. Is there anything more to do here, or can we get this little business over with?

dfeuer marked an inline comment as done.Aug 28 2018, 4:10 PM
dfeuer updated this revision to Diff 17850.Aug 28 2018, 4:11 PM
  • Remove outdated comment
dfeuer updated this revision to Diff 17859.Aug 29 2018, 10:12 AM
  • Don't bother trying to deprecate the C stuff. It seems tricky.
dfeuer updated this revision to Diff 17861.Aug 29 2018, 10:33 AM
  • Silly syntax error
dfeuer edited the summary of this revision. (Show Details)Aug 29 2018, 10:38 AM
dfeuer updated this revision to Diff 17862.Aug 29 2018, 10:52 AM
  • Formatting wibble
bgamari accepted this revision.EditedAug 29 2018, 3:08 PM

Very nice cleanup! Looks good.

docs/users_guide/ffi-chap.rst
286

Thanks for the documentation!

This revision is now accepted and ready to land.Aug 29 2018, 3:08 PM
dfeuer edited the summary of this revision. (Show Details)Aug 29 2018, 3:08 PM

That being said, a mention in the release notes would be good.

dfeuer updated this revision to Diff 17863.Aug 29 2018, 3:17 PM
  • Update release notes
This revision was automatically updated to reflect the committed changes.