Page MenuHomePhabricator

Use a non-recursive mutex in GsymCreator.
ClosedPublic

Authored by simon.giesecke on May 14 2021, 5:18 AM.

Details

Summary

There doesn't seem to be a need to support recursive locking,
and a recursive mutex is unnecessarily inefficient.

Diff Detail

Event Timeline

simon.giesecke created this revision.May 14 2021, 5:18 AM
simon.giesecke requested review of this revision.May 14 2021, 5:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2021, 5:18 AM
simon.giesecke added a comment.EditedMay 14 2021, 5:36 AM

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue here. I was wondering if we could avoid locking the mutex here, but using thread-local tables until finalisation does not seem to be feasible since any thread might insert the same string IIUC and we want to have unique IDs at this point. A lock-free map might be an alternative here, though?

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue

Could perhaps start by moving the CachedHashStringRef construction outside the lock?

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue

Could perhaps start by moving the CachedHashStringRef construction outside the lock?

The "Copy" argument is usually set to "false" so it won't end up copying the string. We only have to back the string up if the DWARF information had no mangled name for an entry and we and up calculating a fully qualified name by traversing the DWARF. This is rare though.

clayborg accepted this revision.May 14 2021, 2:43 PM

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue here. I was wondering if we could avoid locking the mutex here, but using thread-local tables until finalisation does not seem to be feasible since any thread might insert the same string IIUC and we want to have unique IDs at this point. A lock-free map might be an alternative here, though?

I am open to ideas to speed things up, but many things rely on strings being correctly uniqued: strings for function names and inlined function names, files in the file tables (which consist of two string: directory + basename).

This revision is now accepted and ready to land.May 14 2021, 2:43 PM

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue

Could perhaps start by moving the CachedHashStringRef construction outside the lock?

Yes, makes sense. I have missed that opportunity somehow in D102484

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue here. I was wondering if we could avoid locking the mutex here, but using thread-local tables until finalisation does not seem to be feasible since any thread might insert the same string IIUC and we want to have unique IDs at this point. A lock-free map might be an alternative here, though?

I am open to ideas to speed things up, but many things rely on strings being correctly uniqued: strings for function names and inlined function names, files in the file tables (which consist of two string: directory + basename).

Sure, that's a property that must be maintained.

GSYM conversion still spends a significant time for locking in GsymCreator::insertString even with only 8 threads, so lock contention seems to be generally an issue

Could perhaps start by moving the CachedHashStringRef construction outside the lock?

The "Copy" argument is usually set to "false" so it won't end up copying the string. We only have to back the string up if the DWARF information had no mangled name for an entry and we and up calculating a fully qualified name by traversing the DWARF. This is rare though.

The "Copy" case might be rare, but the hash needs to be calculated regardless of whether "Copy" is true.

@clayborg Can you land the four patches again for me? Thanks!

Or should I rather request commit access? I saw the other patch you landed now mentions you both as the author and the reviewer, which seems a bit odd.

I would go ahead and request commit access, but at least there is the Phabricator link in the commit that shows that you authored the diff. This is the main reason you want to get commit access, so you everyone can easily see who did the patches.

simon.giesecke edited the summary of this revision. (Show Details)

Update using arc

This revision was automatically updated to reflect the committed changes.