[Runtime] Switch MetadataCache to ConcurrentReadableHashMap. #34307

mikeash · 2020-10-14T18:29:09Z

Use StableAddressConcurrentReadableHashMap for now. MetadataCacheEntry's methods for awaiting a particular state assume a stable address, where it will repeatedly examine this in a loop while waiting on a condition variable, so we give it a stable address to accommodate that. This code can be changed to perform the necessary table lookup each time through the loop instead, and thus allow us to use a plain ConcurrentReadableHashMap and avoid the extra indirection, and I'll take care of that in a followup.

rdar://problem/70220660

mikeash · 2020-10-14T18:29:18Z

@swift-ci please benchmark

rjmccall

So the actual entry in the hashtable now is an EntryType*, right? Or is it indirected an extra step for some reason?

rjmccall · 2020-10-15T03:30:29Z

stdlib/public/runtime/Metadata.cpp

+    }
+
+    Key key{value.Data.NumElements, elements.data(), value.Data.Labels};
+    return hash_value(key);


Surely there's a way to do this without having to copy the elements into a local array. You could use a TransformRange to project out the Metadata* for hash_value().

I couldn't find a nice way to do it, but I didn't know we had TransformRange. That looks nice and simple, I'll do that.

Note that entry hashes are only used when resizing the table, so it's not a performance impact for lookups. But it's still definitely worth doing better.

rjmccall · 2020-10-15T03:34:02Z

stdlib/public/runtime/MetadataCache.h

@@ -106,17 +106,14 @@ struct ConcurrencyControl {
  ConcurrencyControl() = default;
 };

-template <class EntryType, uint16_t Tag, bool ProvideDestructor = true>
+template <class EntryType, uint16_t Tag>


Is there just never a situation where we care about destructors for these?

I couldn't find any. ConcurrentMap doesn't support removal. It does support destroying the entire tree, but all uses of it just create it and let it live forever. I assume this was left over from a much earlier era when it was thought that these caches might not be permanent, or metadata might not be immortal.

Mostly I was reluctant to write a class that might be useless in other contexts.

Ah, that'll do it too.

mikeash · 2020-10-15T14:11:35Z

So the actual entry in the hashtable now is an EntryType*, right? Or is it indirected an extra step for some reason?

That's right. The wrapper forwards key matching operations, but it's ultimately just a pointer, so there's just one level of indirection. As I mention in the PR description, I'd like to even get rid of that, but it's a bigger job.

rjmccall · 2020-10-15T19:08:22Z

So the actual entry in the hashtable now is an EntryType*, right? Or is it indirected an extra step for some reason?

That's right. The wrapper forwards key matching operations, but it's ultimately just a pointer, so there's just one level of indirection. As I mention in the PR description, I'd like to even get rid of that, but it's a bigger job.

I think the entries usually have the metadata inline, and the metadata address isn't allowed to change, so I'm not sure how you could do that.

mikeash · 2020-10-15T20:19:41Z

The entries store a pointer:

  using ValueType = Metadata *;
...
  /// Valid if TrackingInfo.getState() >= PrivateMetadataState::Abstract.
  ValueType Value;

I think the entry can be created before the size is known, so it has to be separate.

rjmccall · 2020-10-16T04:13:26Z

The entries store a pointer:

It might be an interior pointer. It is for TupleCacheEntry, at least, and I think it is for some of the others as well. And I'm not sure you can safely copy a MetadataCacheEntryBase — you'll need to do serious surgery, at least.

mikeash · 2020-10-16T14:39:40Z

Ah, I didn't catch that it could be an interior pointer! I'm OK with major surgery to improve the cases that can benefit from it, but I will keep in mind that it won't be easy. I'll keep my eye out for better approaches, too.

In the meantime, I need to figure out these benchmark regressions before I land this one....

mikeash · 2020-10-23T19:59:47Z

@swift-ci please benchmark

mikeash · 2020-10-23T20:00:09Z

Playing with some perf improvements to the hashmap code. I'll make them a separate PR if they pan out.

mikeash · 2020-10-28T21:12:39Z

I changed that copying hash_value function to use TransformRange instead. I also got the performance up to par by adding a cache for the most recently looked up type and doing a quick check for a repeated lookup before doing a full lookup, like ConcurrentMap does.

mikeash · 2020-10-28T21:12:47Z

@swift-ci please test

swift-ci · 2020-10-28T21:28:10Z

Build failed
Swift Test OS X Platform
Git Sha - 1b59c5284f7f3ca947ddbdf3beb0e781747bcb3b

mikeash · 2020-10-29T16:25:42Z

The new table spills out of the available space when targeting 32-bit. Working out how to shrink it further...

Use StableAddressConcurrentReadableHashMap for now. MetadataCacheEntry's methods for awaiting a particular state assume a stable address, where it will repeatedly examine `this` in a loop while waiting on a condition variable, so we give it a stable address to accommodate that. This code can be changed to perform the necessary table lookup each time through the loop instead, and thus allow us to use a plain ConcurrentReadableHashMap and avoid the extra indirection, and I'll take care of that in a followup. rdar://problem/70220660

…ntry and check that before doing a more expensive lookup.

…e it to get LockingConcurrentMapStorage to fit into the available space on 32-bit.

mikeash · 2020-10-30T16:40:31Z

Shrank it using some indirection. We really should adopt os_unfair_lock on Darwin as much as possible, though. It's nice and small and avoids this whole issue.

mikeash · 2020-10-30T16:41:15Z

@swift-ci please test

mikeash · 2020-10-30T20:26:10Z

@swift-ci please test

swift-ci · 2020-10-31T00:13:29Z

Build failed
Swift Test OS X Platform
Git Sha - cf53ad2

mikeash · 2020-11-02T15:53:34Z

@swift-ci please test os x platform

mikeash · 2020-11-02T20:11:17Z

@rjmccall Any other changes or comments before I merge?

mikeash · 2020-11-04T15:41:43Z

@swift-ci please test

mikeash · 2020-11-04T15:41:50Z

Re-testing to make sure nothing broke since last week.

swift-ci · 2020-11-04T17:40:04Z

Build failed
Swift Test OS X Platform
Git Sha - cf53ad2

mikeash · 2020-11-04T20:09:24Z

The OS X test failure is unrelated. Pushing the Big Red Button.

* 'main' of github.com:apple/swift: [Runtime] Switch MetadataCache to ConcurrentReadableHashMap. (swiftlang#34307)

mikeash requested review from jckarter, rjmccall and tbkka October 14, 2020 18:29

rjmccall reviewed Oct 15, 2020

View reviewed changes

mikeash force-pushed the metadatacache-concurrentreadablehashmap branch from 343a15d to 9174785 Compare October 28, 2020 20:02

swiftlang deleted a comment from swift-ci Oct 28, 2020

mikeash requested a review from rjmccall October 28, 2020 21:09

mikeash force-pushed the metadatacache-concurrentreadablehashmap branch from 9174785 to 1b59c52 Compare October 28, 2020 21:11

mikeash added 3 commits October 30, 2020 11:14

Have StableAddressConcurrentReadableHashMap remember the last found e…

f08a091

…ntry and check that before doing a more expensive lookup.

Make a SmallMutex type that stores the mutex data out of line, and us…

cf53ad2

…e it to get LockingConcurrentMapStorage to fit into the available space on 32-bit.

mikeash force-pushed the metadatacache-concurrentreadablehashmap branch from 0a08cae to cf53ad2 Compare October 30, 2020 16:36

mikeash merged commit 12d1147 into swiftlang:main Nov 4, 2020

ainu-bot added a commit to google/swift that referenced this pull request Nov 4, 2020

Merge branch 'main' of github.com:apple/swift into tensorflow-stage

6b63fb4

* 'main' of github.com:apple/swift: [Runtime] Switch MetadataCache to ConcurrentReadableHashMap. (swiftlang#34307)

[Runtime] Switch MetadataCache to ConcurrentReadableHashMap. #34307

[Runtime] Switch MetadataCache to ConcurrentReadableHashMap. #34307

Uh oh!

Conversation

mikeash commented Oct 14, 2020

Uh oh!

mikeash commented Oct 14, 2020

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

rjmccall Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

mikeash Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

rjmccall Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

mikeash Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

rjmccall Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

mikeash Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

mikeash commented Oct 15, 2020

Uh oh!

rjmccall commented Oct 15, 2020

Uh oh!

mikeash commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjmccall commented Oct 16, 2020

Uh oh!

mikeash commented Oct 16, 2020

Uh oh!

mikeash commented Oct 23, 2020

Uh oh!

mikeash commented Oct 23, 2020

Uh oh!

mikeash commented Oct 28, 2020

Uh oh!

mikeash commented Oct 28, 2020

Uh oh!

swift-ci commented Oct 28, 2020

Uh oh!

mikeash commented Oct 29, 2020

Uh oh!

mikeash commented Oct 30, 2020

Uh oh!

mikeash commented Oct 30, 2020

Uh oh!

mikeash commented Oct 30, 2020

Uh oh!

swift-ci commented Oct 31, 2020

Uh oh!

mikeash commented Nov 2, 2020

Uh oh!

mikeash commented Nov 2, 2020

Uh oh!

mikeash commented Nov 4, 2020

Uh oh!

mikeash commented Nov 4, 2020

Uh oh!

swift-ci commented Nov 4, 2020

Uh oh!

mikeash commented Nov 4, 2020

Uh oh!

Uh oh!

mikeash commented Oct 15, 2020 •

edited

Loading