Use std::unordered_map instead of std::map in APZCTreeManager

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kats, Assigned: silverskinx, Mentored)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [gfx-noted] [lang=c++] [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

In a couple of places in APZCTreeManager [1][2], we use a std::map with a ScrollableLayerGuid key. (I'm adding another such instance of this in bug 1364525). Since lookups are faster in an std::unordered_map than a std::map, we should switch to using unordered:map. This will require defining a hash function for ScrollableLayerGuid as well.

[1] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/gfx/layers/apz/src/APZCTreeManager.cpp#89
[2] http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/gfx/layers/apz/src/APZCTreeManager.h#545
Summary: Use std::unordered_map instead of std::map in APZCTreeManager.cpp → Use std::unordered_map instead of std::map in APZCTreeManager
Priority: -- → P3
Whiteboard: [gfx-noted]
Mentor: botond
Whiteboard: [gfx-noted] → [gfx-noted] [lang=c++] [good first bug]
Assignee

Comment 1

2 years ago
I'd like to take a stab at this. 
I would have to change all instances of std::map to std::unordered_map in APZCTreeManager and then create a hash function to preserve std::map's mapping, right?
(In reply to silverskinx from comment #1)
> I'd like to take a stab at this.

Great, thanks for your interest!

The first step is to build Firefox as described here [1].

> I would have to change all instances of std::map to std::unordered_map in
> APZCTreeManager and then create a hash function to preserve std::map's
> mapping, right?

Yep, that's the idea. (Note that the order of the elements will not be the same as with std::map, but that's fine, because we just use the map for lookups. The hash function is needed for std::unordered_map to compile at all with a ScrollableLayerGuid key.)

There are two ways to define the hash function:

  1) Specialize std::hash (the default hash function) for ScrollableLayerGuid.

  2) Define our own class to serve as the hash function. In this case, the
     name of this class needs to be passed as the third template parameter
     to std::unordered_map.

I'll leave it up to you which one you go with, it doesn't really make much difference.

Let me know if you have any questions / run into any issues!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
Assignee

Comment 3

2 years ago
So I made a class for the hash function, but I am not sure where it should go, should I put the class in HashFunctions.h?
(In reply to silverskinx from comment #3)
> So I made a class for the hash function, but I am not sure where it should
> go, should I put the class in HashFunctions.h?

Since our hash function is specific to the ScrollableLayerGuid class, it makes the most sense to define it in the same file that defines ScrollableLayerGuid itself, which is gfx/layers/FrameMetrics.h.
Comment hidden (mozreview-request)

Comment 6

2 years ago
Posted patch Fix of a bug 1367062. (obsolete) — Splinter Review

Comment 7

2 years ago
Oh, sorry, I didn't notice it was already done.
Assignee: nobody → silverskinx

Comment 8

2 years ago
mozreview-review
Comment on attachment 8871927 [details]
Bug 1367062 - Change std::map to std::unordered_map in APZCTreeManager

https://reviewboard.mozilla.org/r/143420/#review147184

Thanks, this looks good! I didn't realize ScrollableLayerGuid already had a Hash() method, that certainly made things easy :)

There is one more use of std::map with a ScrollableLayerGuid key in APZCTreeManager.cpp that should be converted:

http://searchfox.org/mozilla-central/rev/c195cc1698da2d3116fd8594db44614bb1304719/gfx/layers/apz/src/APZCTreeManager.cpp#383

(It was just added a few days ago, so it's possible that you were looking at an older version of the file which didn't have it.)
Comment on attachment 8871936 [details] [diff] [review]
Fix of a bug 1367062.

Grigory, thanks for your interest in this bug! As you've noticed, there is already someone working on it, so I would encourage you to find another [good first bug] to work on instead. You can use this tool to find one:

http://www.joshmatthews.net/bugsahoy/

I'm going to mark your patch here as "obsolete" to avoid confusion from there being two patches attached to this bug.
Attachment #8871936 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8871984 [details]
Bug 1367062 - Change std::map to std::unordered_map in APZCTreeManager

https://reviewboard.mozilla.org/r/143530/#review147224

Looks good, thanks!

Could you combine the two patches into one, please? You can do this with "hg histedit" (or "git rebase -i" if you use git).

(In the future, you can use "hg commit --amend" (or "git commit --amend") to add additional changes into an existing commit.)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8871927 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8871984 [details]
Bug 1367062 - Change std::map to std::unordered_map in APZCTreeManager

https://reviewboard.mozilla.org/r/143530/#review147250
Attachment #8871984 - Flags: review+
(In reply to Botond Ballo [:botond] from comment #16)
> Pushed the patch to the Try server for testing:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e5f661fb5b4c46ebb408c61e1a519534bf04df34

Test results look good. I will go ahead and commit this.

Comment 18

2 years ago
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5246b79016cc
Change std::map to std::unordered_map in APZCTreeManager r=botond

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5246b79016cc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
silverskinx, thanks for your contribution here!

If you're interested in working on another bug, you can have a look at the list of other bugs I'm mentoring:

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=mentor%3Dbotond%40mozilla.com

or use this tool to find a new bug to work on:

https://www.joshmatthews.net/bugsahoy/
You need to log in before you can comment on or make changes to this bug.