Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: khuey, Assigned: froydnj)

Tracking

unspecified
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 2 obsolete attachments)

1021 bytes, patch
khuey
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
513 bytes, patch
mak
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.01 KB, patch
dbaron
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
933 bytes, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
4.98 KB, patch
kaie
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.31 KB, patch
bas
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
2.06 KB, patch
jduell
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.28 KB, patch
hsivonen
: review+
smaug
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
1.24 KB, patch
froydnj
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
687 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.26 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
7.94 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
Everything in here is obsolete and nasty and should die.
Is there a obvious (templated?) replacement candidate for the types defined therein?  All the HashSets kinds (save for nsInt32HashSet) are used at least once throughout the codebase.
nsTHashTable<nsFooHashKey> is the replacement.
Patches incoming shortly to remove *HashSet throughout the code base save for nsCheapSets.  I think nsCheapSets could stand to abandon its pointer-twiddling bits and become templatized, but that's a discussion for a separate bug.  (FWIW, there's only one use of nsCheapSets in the whole tree, so I think it'd be reasonable to make them just a little bit bigger.)

The patches are mostly mechanical and fairly tiny; they could be made even tiner with a little more twiddling.  Will explain in a bit.
Created attachment 572955 [details] [diff] [review]
slay nsHashSets in xpcom/base/

Later patches are much nicer with a Contains method in nsTHashtable.

Later patches could also become even nicer with Put/Remove methods; I opted not to add them because they seemed like clutter.
Attachment #572955 - Flags: review?(benjamin)
Created attachment 572956 [details] [diff] [review]
slay nsHashSets in storage/src/

Remove offending bits from gfx/thebes/.
Attachment #572956 - Flags: review?(bas.schouten)
Created attachment 572959 [details] [diff] [review]
slay nsHashSets in layout/

Remove offending bits from netwerk/cache/
Attachment #572959 - Flags: review?(jduell.mcbugs)
Created attachment 572960 [details] [diff] [review]
slay nsHashSets in parser/

Remove offending bits in parser/html/
Attachment #572960 - Flags: review?(bugs)
Created attachment 572962 [details] [diff] [review]
slay nsHashSets in content/

Remove offending bits in content/base/.  Oddly enough, this is just dead #includes.
Attachment #572962 - Flags: review?(bzbarsky)
Created attachment 572963 [details] [diff] [review]
slay nsHashSets in security/

Remove offending bits in security/manager/ssl/.
Attachment #572963 - Flags: review?(kaie)
Attachment #572959 - Attachment description: slay nsHashSets in netwerk/ → slay nsHashSets in layout/
Attachment #572959 - Flags: review?(jduell.mcbugs) → review?(dbaron)
Attachment #572956 - Attachment description: slay nsHashSets in gfx/thebes/ → slay nsHashSets in storage/src/
Attachment #572956 - Flags: review?(bas.schouten) → review?(mak77)
Attachment #572955 - Attachment description: add a Contains method to nsTHashtable → slay nsHashSets in xpcom/base/
Attachment #572955 - Flags: review?(benjamin) → review?(khuey)
Comment on attachment 572960 [details] [diff] [review]
slay nsHashSets in parser/

This patch certainly isn't anything about parser/
Attachment #572960 - Flags: review?(bugs)
Created attachment 572967 [details] [diff] [review]
add a Contains method to nsTHashtable

Later patches are much nicer with a Contains method in nsTHashtable.

Later patches could also become even nicer with Put/Remove methods; I opted not to add them because they seemed like clutter.

(Now with the correct attachment!)
Attachment #572967 - Flags: review?(benjamin)
Created attachment 572968 [details] [diff] [review]
slay nsHashSets in gfx/thebes/

Remove offending bits in gfx/thebes/
Attachment #572968 - Flags: review?(bas.schouten)
Created attachment 572969 [details] [diff] [review]
slay nsHashSets in netwerk/

Remove offending bits from netwerk/cache/.
Attachment #572969 - Flags: review?(jduell.mcbugs)
Created attachment 572971 [details] [diff] [review]
slay nsHashSets in parser/

Remove offending bits in parser/html/.
Attachment #572960 - Attachment is obsolete: true
Attachment #572971 - Flags: review?(bugs)
Assignee: nobody → nfroyd
Comment on attachment 572971 [details] [diff] [review]
slay nsHashSets in parser/

I think Henri should look at all the changes happening under parser/
Attachment #572971 - Flags: review?(hsivonen)
Attachment #572971 - Flags: review?(bugs)
Attachment #572971 - Flags: review+
OK, I think everything's in the right place now.  I hate uploading patch series to Bugzilla!
I'd really much rather have a nicer set API on top of nsTHashtable.  I wrote one once, but never found an excuse to land it, because both times I used it I ended up scrapping that patch.  But it's here:

https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/dba69109eff4/hash-set

Comment 18

6 years ago
Comment on attachment 572962 [details] [diff] [review]
slay nsHashSets in content/

r=me
Attachment #572962 - Flags: review?(bzbarsky) → review+

Updated

6 years ago
Attachment #572956 - Flags: review?(mak77) → review+
Attachment #572955 - Flags: review?(khuey) → review+
Comment on attachment 572968 [details] [diff] [review]
slay nsHashSets in gfx/thebes/

Review of attachment 572968 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me. Not sure if I'm the very best person to review this but I doubt it matters.
Attachment #572968 - Flags: review?(bas.schouten) → review+
Attachment #572971 - Flags: review?(hsivonen) → review+
Comment on attachment 572967 [details] [diff] [review]
add a Contains method to nsTHashtable

I think this would be easier to read as:

return !!GetEntry(aKey);

r=me like that
Attachment #572967 - Flags: review?(benjamin) → review+
Attachment #572969 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 572959 [details] [diff] [review]
slay nsHashSets in layout/

r=dbaron
Attachment #572959 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] from comment #17)
> I'd really much rather have a nicer set API on top of nsTHashtable.  I wrote
> one once, but never found an excuse to land it, because both times I used it
> I ended up scrapping that patch.  But it's here:
> 
> https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/
> dba69109eff4/hash-set

I filed this as bug 708901.
kaie, review ping?

Comment 24

6 years ago
Comment on attachment 572963 [details] [diff] [review]
slay nsHashSets in security/

r=kaie
Attachment #572963 - Flags: review?(kaie) → review+
Is this ready to go?
I need to fixup nsTHashtable per comment 20.  Then this batch of patches, at least, are ready to go in.
Depends on: 722689
Duplicate of this bug: 405988
Created attachment 599646 [details] [diff] [review]
part 1 - add a Contains method to nsTHashtable

Updating to address review comments.  Carrying over r+.
Attachment #572967 - Attachment is obsolete: true
Attachment #599646 - Flags: review+
Please apply "part 1" first; the remaining patches are independent of each other.

Also, there's more work to be done (nsHashSets* needs to be removed once dependent bugs are fixed), so please leave open after the m-c merge.  Thanks!
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [please leave open after merge]
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aaa5f1f685e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f66366413f66
https://hg.mozilla.org/integration/mozilla-inbound/rev/a94b06b56773
https://hg.mozilla.org/integration/mozilla-inbound/rev/6735b4f3a4c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a41cc70ce9c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b85b6482eb65
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f98fc956d25
https://hg.mozilla.org/integration/mozilla-inbound/rev/2651ff8e19e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9a34063fb2
Keywords: checkin-needed
Attachment #572955 - Flags: checkin+
Attachment #572956 - Flags: checkin+
Attachment #572959 - Flags: checkin+
Attachment #572962 - Flags: checkin+
Attachment #572963 - Flags: checkin+
Attachment #572968 - Flags: checkin+
Attachment #572969 - Flags: checkin+
Attachment #572971 - Flags: checkin+
Attachment #599646 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/7aaa5f1f685e
https://hg.mozilla.org/mozilla-central/rev/f66366413f66
https://hg.mozilla.org/mozilla-central/rev/a94b06b56773
https://hg.mozilla.org/mozilla-central/rev/6735b4f3a4c4
https://hg.mozilla.org/mozilla-central/rev/a41cc70ce9c2
https://hg.mozilla.org/mozilla-central/rev/b85b6482eb65
https://hg.mozilla.org/mozilla-central/rev/3f98fc956d25
https://hg.mozilla.org/mozilla-central/rev/2651ff8e19e9
https://hg.mozilla.org/mozilla-central/rev/0c9a34063fb2
Blocks: 730825
Created attachment 600918 [details] [diff] [review]
Slay nsHashSets in gfx/thebes/, round 2

Just an unused #include.
Attachment #600918 - Flags: review?(jmuizelaar)
Created attachment 600920 [details] [diff] [review]
Slay nsHashSets in netwerk/protocol/

I think the SPDY work added an nsHashSets usage.
Attachment #600920 - Flags: review?(mcmanus)
Created attachment 600922 [details] [diff] [review]
last part - delete nsHashSets code
Attachment #600922 - Flags: review?(benjamin)
Attachment #600918 - Flags: review?(jmuizelaar) → review+
Attachment #600920 - Flags: review?(mcmanus) → review+
Attachment #600922 - Flags: review?(benjamin) → review+
Please checkin the netwerk/ and gfx/ parts, then the actual deletion ("last part") patch, otherwise we'll wind up with a broken tree along the way.  Thanks!
Keywords: checkin-needed
Whiteboard: [please leave open after merge]
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0913432b56e
https://hg.mozilla.org/integration/mozilla-inbound/rev/949612a18317
https://hg.mozilla.org/integration/mozilla-inbound/rev/bca0ee06670c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0913432b56e
https://hg.mozilla.org/mozilla-central/rev/949612a18317
https://hg.mozilla.org/mozilla-central/rev/bca0ee06670c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.