Make Rule nameCache thread safe

[keith]

No description provided.

[keith]

Alternatively I'm not sure how much time this optimization saved but we could attempt to remove it instead if that's not too costly.

[keith]

I'm also happy to try some other variants of this optimization such as not splitting on every . if I know what level of performance gains we're looking for

[allevato]

Previously, we had the rule cache but weren't actually updating it, and populating the cache yielded a significant performance improvement (13fd74d#diff-850e7af4319cd80f84d3ffdc821387f341b7cc167465cbb3e3b20d4e9b7fc142) when @dylansturg fixed it.

I don't recall, however, where the time profile mostly showed we were getting hit—it very well could have been the repeated heap allocations for the array result of split. If that was the case, then an improvement would be to do what you alluded to and stop using split, but instead just find the last occurrence of . and take the tail of that string.

If you'd like to explore a couple options and see what the time profiles look like, that'd be great—but I'm also happy to merge this as-is and do that exploration later. WDYT?

[keith]

I did a few tests here (not super scientifically):

1. Tests were based on 842f22a
2. Built with swift build -c release
3. On swift-protobuf 15350c334c7171f81785c709611b347806aae37a
4. Run swift-format lint -r . 2>/dev/null

• With current use of cache + .split: 40s
• No caching + .split: 53s
• With this change to use .split + supporting multithreading: 45s

Then I tried some variants using indexes, it's quite cumbersome especially considering not all strings here contain a ., but the logic I was testing was something like this:

let name = String(describing: self)
guard let dotIndex = name.lastIndex(of: ".") else {
    nameCache[identifier] = name
    return name
}

let splitIndex = name.index(after: dotIndex)
return String(name[splitIndex...])

• With indexes + multithreading: 49s
• With current cache + indexes: 45s

And finally my favorite: with this change + the --parallel flag 6.8s 😄

So overall I think there's a bit of a hit in this in the non-parallel case, but between other string options it does seem to be a fine version of it.

[allevato]

Sorry for the delay! Let's merge this in so it's not blocking the other parallel change, and if we want to do some more rigorous testing and streamline this later, we have more flexibility to do that. But those numbers look good already.

[keith]

Thanks!