Closed Bug 1367062 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kats, Assigned: silverskinx, Mentored)

Details

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

Attachments

(1 file, 2 obsolete files)

Bug 1367062 - Change std::map to std::unordered_map in APZCTreeManager
59 bytes, text/x-review-board-request
botond
: review+
Details 
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]
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
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.
Attached patch Fix of a bug 1367062. (obsolete) — — Splinter Review 

    
    

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

    
    

    
  
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.)

    
    

    
      
  

    
  

        Attachment #8871927 -
        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/#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.

    
    

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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/5246b79016cc
Status: NEW → RESOLVED
Closed: 7 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: