It calls into some slowish CFNetwork code and ends up showing up in profiles.
Created attachment 352888 [details] patch
Created attachment 352890 [details] patch
Comment on attachment 352890 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=352890&action=review > Source/WebCore/platform/mac/PublicSuffixMac.mm:51 > + static NeverDestroyed<HashMap<String, AtomicString, ASCIICaseInsensitiveHash>> cache; Is there really a benefit to using AtomicString here?
> Is there really a benefit to using AtomicString here? At least in theory. Many domains may map into the same top domain. It shouldn't hurt.
Comment on attachment 352890 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=352890&action=review >> Source/WebCore/platform/mac/PublicSuffixMac.mm:51 >> + static NeverDestroyed<HashMap<String, AtomicString, ASCIICaseInsensitiveHash>> cache; > > Is there really a benefit to using AtomicString here? Have you confirmed that this method is not called from multiple threads? I suspect it could be (e.g. for ITP).
Comment on attachment 352890 [details] patch At the very least, it seems we should ASSERT.
Comment on attachment 352890 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=352890&action=review > Source/WebCore/platform/mac/PublicSuffixMac.mm:55 > + cache.get().clear(); Seems very aggressive to clear the whole cache? Why not simply kick one value out like we usually do?
> Seems very aggressive to clear the whole cache? Why not simply kick one > value out like we usually do? I tested and it turns out it is a terrible strategy for this sort of thing at least in the usual 'map.remove(map.begin())' form. Since the table is never rehashed we always end up removing keys that hash to the early part of the table and the rest are stuck forever.
> Have you confirmed that this method is not called from multiple threads? I > suspect it could be (e.g. for ITP). Ok, I'll just switch to String.
(In reply to Antti Koivisto from comment #9) > > Have you confirmed that this method is not called from multiple threads? I > > suspect it could be (e.g. for ITP). > > Ok, I'll just switch to String. Switching to String would not be sufficient as you'd still have HashMap operations from multiple threads. That said, I do not know for sure we have multithreaded access and an assertion would suffice. If we do have multi threaded access though, then I'd suggest using String + Locks.
(In reply to Antti Koivisto from comment #8) > > Seems very aggressive to clear the whole cache? Why not simply kick one > > value out like we usually do? > > I tested and it turns out it is a terrible strategy for this sort of thing > at least in the usual 'map.remove(map.begin())' form. Since the table is > never rehashed we always end up removing keys that hash to the early part of > the table and the rest are stuck forever. That's good to know. Sounds like we may want a method that returns a random iterator inside the HashMap? e.g. HashMap::random() or HashMap::any()
> Switching to String would not be sufficient as you'd still have HashMap > operations from multiple threads. That said, I do not know for sure we have > multithreaded access and an assertion would suffice. If we do have multi > threaded access though, then I'd suggest using String + Locks. Yep. I'll see if it is being used.
> That's good to know. Sounds like we may want a method that returns a random > iterator inside the HashMap? e.g. HashMap::random() or HashMap::any() Or more explicit support for this pattern: HashMap::shrinkRandomlyToSize(size_t)
(In reply to Antti Koivisto from comment #8) > > Seems very aggressive to clear the whole cache? Why not simply kick one > > value out like we usually do? > > I tested and it turns out it is a terrible strategy for this sort of thing > at least in the usual 'map.remove(map.begin())' form. Since the table is > never rehashed we always end up removing keys that hash to the early part of > the table and the rest are stuck forever. Why doesn't the table rehash?
(In reply to Geoffrey Garen from comment #14) > (In reply to Antti Koivisto from comment #8) > > > Seems very aggressive to clear the whole cache? Why not simply kick one > > > value out like we usually do? > > > > I tested and it turns out it is a terrible strategy for this sort of thing > > at least in the usual 'map.remove(map.begin())' form. Since the table is > > never rehashed we always end up removing keys that hash to the early part of > > the table and the rest are stuck forever. > > Why doesn't the table rehash? My understanding is that we only rehash when shrinking / expanding the HashTable capacity. In this kind of pattern, we have a max entries (in this case 100) so we never need to expand / shrink. We reach 100 entries, remove HashTable::begin() then add the new entry.
(In reply to Chris Dumez from comment #15) > (In reply to Geoffrey Garen from comment #14) > > (In reply to Antti Koivisto from comment #8) > > > > Seems very aggressive to clear the whole cache? Why not simply kick one > > > > value out like we usually do? > > > > > > I tested and it turns out it is a terrible strategy for this sort of thing > > > at least in the usual 'map.remove(map.begin())' form. Since the table is > > > never rehashed we always end up removing keys that hash to the early part of > > > the table and the rest are stuck forever. > > > > Why doesn't the table rehash? > > My understanding is that we only rehash when shrinking / expanding the > HashTable capacity. In this kind of pattern, we have a max entries (in this > case 100) so we never need to expand / shrink. We reach 100 entries, remove > HashTable::begin() then add the new entry. I see. If we always remove one and add one, then the table never rehashes. Yeah, an explicit idiom to select a random entry would be nice.
Created attachment 352960 [details] patch
Comment on attachment 352960 [details] patch Attachment 352960 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9703747 Number of test failures exceeded the failure limit.
Created attachment 352965 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 352967 [details] with locking
Created attachment 352969 [details] with locking
We should update the code to use https://bugs.webkit.org/show_bug.cgi?id=190814 when it lands.
Comment on attachment 352969 [details] with locking Clearing flags on attachment: 352969 Committed r237357: <https://trac.webkit.org/changeset/237357>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45492955>
After the changes in https://trac.webkit.org/changeset/237357/webkit The API test is now failing Log: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/13555/steps/run-api-tests/logs/stdio build: https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK1%20%28Tests%29/builds/13555 Failed TestWebKitAPI.PublicSuffix.TopPrivatelyControlledDomain /Volumes/Data/slave/sierra-release/build/Tools/TestWebKitAPI/Tests/WebCore/PublicSuffix.cpp:159 Expected equality of these values: String() Which is: topPrivatelyControlledDomain("") Which is:
Reverted r237357 for reason: API test is now failing on all platforms. Committed r237366: <https://trac.webkit.org/changeset/237366>
Created attachment 353026 [details] patch
https://trac.webkit.org/changeset/237379/webkit