59 bytes, text/x-review-board-request
In a couple of places in APZCTreeManager , 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.  http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/gfx/layers/apz/src/APZCTreeManager.cpp#89  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] → [gfx-noted] [lang=c++] [good first bug]
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 . > 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!  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
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.
Oh, sorry, I didn't notice it was already done.
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 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 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+
Thanks! Pushed the patch to the Try server for testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5f661fb5b4c46ebb408c61e1a519534bf04df34
(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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/5246b79016cc Change std::map to std::unordered_map in APZCTreeManager r=botond
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
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.