Bug 154352 - Extend HashCountedSet with a method to efficiently set the count of an entry
Summary: Extend HashCountedSet with a method to efficiently set the count of an entry
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on: 154438
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-17 12:28 PST by Brent Fulgham
Modified: 2016-02-18 23:42 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.27 KB, patch)
2016-02-17 14:20 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (29.37 KB, patch)
2016-02-18 10:22 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (30.15 KB, patch)
2016-02-18 16:03 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (With Windows fixes) (29.34 KB, patch)
2016-02-18 16:06 PST, Brent Fulgham
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-02-17 12:28:13 PST
When working on Bug 153575, I had to add some silly code like the following:

        // FIXME: Create a HashCountedSet method to do this efficiently
        for (unsigned i = 0; i < count; ++i)
            hashCountedSet.add(origin);

This bug adds a method to support seeding the HashCountedSet with the appropriate count.
Comment 1 Brent Fulgham 2016-02-17 14:20:14 PST
Created attachment 271590 [details]
Patch
Comment 2 Geoffrey Garen 2016-02-17 16:47:46 PST
Comment on attachment 271590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271590&action=review

> Source/WTF/wtf/HashCountedSet.h:166
> +    inline typename HashCountedSet<Value, HashFunctions, Traits>::AddResult HashCountedSet<Value, HashFunctions, Traits>::addWithInitialCount(const ValueType& value, unsigned initialCount)
> +    {
> +        return m_impl.add(value, initialCount);
> +    }

This is slightly wrong -- and also a bit surprising.

If the set already contains the object, this will do nothing.

You should either call this function set, and have it call through to set, or you should call it add, and have it add 0, and then increment the resulting iterator by count, similar to the code above that adds 1.
Comment 3 Brent Fulgham 2016-02-17 16:57:55 PST
Comment on attachment 271590 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271590&action=review

It's also missing tests. I'm fixing both issues.

>> Source/WTF/wtf/HashCountedSet.h:166
>> +    }
> 
> This is slightly wrong -- and also a bit surprising.
> 
> If the set already contains the object, this will do nothing.
> 
> You should either call this function set, and have it call through to set, or you should call it add, and have it add 0, and then increment the resulting iterator by count, similar to the code above that adds 1.

OK!
Comment 4 Brent Fulgham 2016-02-17 22:17:36 PST
While building a test suite for this class, I noticed that many of the data types accepted by HashMap are improperly treated by this wrapper class. I'll update this class to provide move operators and an initializer list constructor to support the same features as its core implementation class.
Comment 5 Brent Fulgham 2016-02-18 10:22:52 PST
Created attachment 271667 [details]
Patch
Comment 6 WebKit Commit Bot 2016-02-18 10:24:38 PST
Attachment 271667 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/HashCountedSet.h:289:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:296:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:303:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:310:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:317:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Brent Fulgham 2016-02-18 10:25:40 PST
(In reply to comment #6)
> Attachment 271667 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WTF/wtf/HashCountedSet.h:289:  This { should be at the end of
> the previous line  [whitespace/braces] [4]
> ERROR: Source/WTF/wtf/HashCountedSet.h:296:  This { should be at the end of
> the previous line  [whitespace/braces] [4]
> ERROR: Source/WTF/wtf/HashCountedSet.h:303:  This { should be at the end of
> the previous line  [whitespace/braces] [4]
> ERROR: Source/WTF/wtf/HashCountedSet.h:310:  This { should be at the end of
> the previous line  [whitespace/braces] [4]
> ERROR: Source/WTF/wtf/HashCountedSet.h:317:  This { should be at the end of
> the previous line  [whitespace/braces] [4]
> Total errors found: 5 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

These template declarations match the style of the rest of the file. I'm not sure if we want to use a different style for just those lines.
Comment 8 Brent Fulgham 2016-02-18 16:01:30 PST
It looks like Visual Studio doesn't like the 'template' syntax. I've got a build fix on Windows, which I need to re-test on Mac.
Comment 9 Brent Fulgham 2016-02-18 16:03:58 PST
Created attachment 271714 [details]
Patch
Comment 10 Brent Fulgham 2016-02-18 16:06:20 PST
Created attachment 271715 [details]
Patch (With Windows fixes)
Comment 11 WebKit Commit Bot 2016-02-18 16:07:19 PST
Attachment 271715 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/HashCountedSet.h:289:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:296:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:303:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:310:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashCountedSet.h:317:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Geoffrey Garen 2016-02-18 16:17:48 PST
Comment on attachment 271715 [details]
Patch (With Windows fixes)

r=me
Comment 13 Brent Fulgham 2016-02-18 17:14:02 PST
Committed r196791: <http://trac.webkit.org/changeset/196791>
Comment 14 Csaba Osztrogonác 2016-02-18 22:34:56 PST
(In reply to comment #13)
> Committed r196791: <http://trac.webkit.org/changeset/196791>

It broke tge Windows build.
Comment 15 Alex Christensen 2016-02-18 22:48:36 PST
Comment on attachment 271715 [details]
Patch (With Windows fixes)

View in context: https://bugs.webkit.org/attachment.cgi?id=271715&action=review

> Source/WTF/wtf/HashCountedSet.h:290
> +        return m_impl.template find(value);

Visual Studio doesn't like this syntax.

> Source/WTF/wtf/HashCountedSet.h:321
> -} // namespace khtml
> +} // namespace WTF

The last namespace khtml comment is gone :(
Comment 16 WebKit Commit Bot 2016-02-18 22:50:46 PST
Re-opened since this is blocked by bug 154438
Comment 17 Brent Fulgham 2016-02-18 23:16:13 PST
(In reply to comment #15)
> Comment on attachment 271715 [details]
> Patch (With Windows fixes)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271715&action=review
> 
> > Source/WTF/wtf/HashCountedSet.h:290
> > +        return m_impl.template find(value);
> 
> Visual Studio doesn't like this syntax.

Ugh! I had this fix on my Windows machine, but I landed from my Mac. >:-(
Comment 18 Brent Fulgham 2016-02-18 23:22:49 PST
Committed r196802: <http://trac.webkit.org/changeset/196802>
Comment 19 Brent Fulgham 2016-02-18 23:32:05 PST
Note: This might need a clean build. It looks like the "template" error was showing after the rollout, so it's possible that my recent fix will also result in at least one "stale" build failure.
Comment 20 Joseph Pecoraro 2016-02-18 23:42:01 PST
Comment on attachment 271715 [details]
Patch (With Windows fixes)

View in context: https://bugs.webkit.org/attachment.cgi?id=271715&action=review

>> Source/WTF/wtf/HashCountedSet.h:321
>> +} // namespace WTF
> 
> The last namespace khtml comment is gone :(

!