Last Comment Bug 737239 - Use bit twiddling for gfxUtils::NextPowerOfTwo on non-arm builds
: Use bit twiddling for gfxUtils::NextPowerOfTwo on non-arm builds
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla14
Assigned To: Nick Cameron [:nrc]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-19 16:09 PDT by Nick Cameron [:nrc]
Modified: 2012-04-12 10:11 PDT (History)
7 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.81 KB, patch)
2012-03-19 16:10 PDT, Nick Cameron [:nrc]
gwright: review+
Details | Diff | Review
updated patch (2.04 KB, patch)
2012-03-20 12:53 PDT, Nick Cameron [:nrc]
gwright: review+
Details | Diff | Review
updated patch (2.29 KB, patch)
2012-03-20 19:22 PDT, Nick Cameron [:nrc]
bas: review+
Details | Diff | Review

Description Nick Cameron [:nrc] 2012-03-19 16:09:10 PDT

    
Comment 1 Nick Cameron [:nrc] 2012-03-19 16:10:34 PDT
Created attachment 607355 [details] [diff] [review]
patch
Comment 2 George Wright (:gw280) (:gwright) 2012-03-20 09:29:20 PDT
Comment on attachment 607355 [details] [diff] [review]
patch

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

Looks fine otherwise.

::: gfx/thebes/gfxUtils.h
@@ +210,1 @@
>      return 1 << (32 - CountLeadingZeroes(aNumber - 1));

Perhaps it's best to just call __builtin_clz directly here instead of having two separate ifdefs.
Comment 3 Nick Cameron [:nrc] 2012-03-20 12:53:22 PDT
Created attachment 607671 [details] [diff] [review]
updated patch

Inlined the clz call.

I've asked for re-review because I've removed the Android copyright notice (doesn't seem to cover anything now) and wanted to check that was OK.
Comment 4 George Wright (:gw280) (:gwright) 2012-03-20 13:17:25 PDT
Comment on attachment 607671 [details] [diff] [review]
updated patch

lgtm
Comment 5 George Wright (:gw280) (:gwright) 2012-03-20 13:19:25 PDT
(In reply to Nick Cameron from comment #3)
> Created attachment 607671 [details] [diff] [review]
> updated patch
> 
> Inlined the clz call.
> 
> I've asked for re-review because I've removed the Android copyright notice
> (doesn't seem to cover anything now) and wanted to check that was OK.

Oops only just noticed this. The copyright should still apply to the ARM optimised call that uses __builtin_clz, as well as the IsPowerOfTwo code. However both are very generic so the copyright notice may be moot.

Also - just noticed that your indentation is set to 4 spaces for the ARM case and 2 spaces otherwise. I think it should be 4 for all.
Comment 6 Nick Cameron [:nrc] 2012-03-20 19:22:01 PDT
Created attachment 607836 [details] [diff] [review]
updated patch

Fixed indent, undeleted copyright notice.
Comment 7 Nick Cameron [:nrc] 2012-03-22 19:45:10 PDT
Successfully navigated push to Try: https://tbpl.mozilla.org/?tree=Try&rev=43826350ded4
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-04-09 15:45:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e54c650e8748

George, you are not currently listed as a Graphics peer on the wiki. Please update that.
https://wiki.mozilla.org/Modules/Core

Also, is there anything that can be tested here? Or do existing tests already cover it?
Comment 9 Jeff Gilbert [:jgilbert] 2012-04-09 16:26:35 PDT
(In reply to Ryan VanderMeulen from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e54c650e8748
> 
> George, you are not currently listed as a Graphics peer on the wiki. Please
> update that.
> https://wiki.mozilla.org/Modules/Core

Yeah, that doesn't seem real up-to-date. For some reason Canvas 2D/WebGL was down in layout, so I pulled it up to graphics.
Comment 10 George Wright (:gw280) (:gwright) 2012-04-09 17:48:04 PDT
(In reply to Ryan VanderMeulen from comment #8)
> George, you are not currently listed as a Graphics peer on the wiki. Please
> update that.
> https://wiki.mozilla.org/Modules/Core

That'd be because I'm not one :)
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-04-09 17:50:00 PDT
errr...so why are patches being checked in on your r+?
Comment 12 Nick Cameron [:nrc] 2012-04-09 18:03:05 PDT
Ah, sorry, my bad, still fairly new to this, didn't realise I needed a review from a graphics peer to check in.
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-04-09 18:07:29 PDT
Patches *always* need review from a module peer before checking in. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/939ec96b6b38
Comment 14 Matt Woodrow (:mattwoodrow) 2012-04-09 18:20:26 PDT
We haven't been enforcing this at all for the graphics module at all. The current list of peers in that document is considerably out of date - Though it may be technically correct since new peers haven't been discussed at all to my knowledge.

I think this can probably be relanded, considering that the majority of recent gfx work would fall under the same classification.
Comment 15 Bas Schouten (:bas.schouten) 2012-04-09 18:35:35 PDT
Comment on attachment 607836 [details] [diff] [review]
updated patch

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

Arguably the graphics peers list is outdated. Meh. I've been asked to review lots of things a little outside gfx as well *shrugs*. I'm listed as a GFx peer though! Look at that!
Comment 16 Jeff Gilbert [:jgilbert] 2012-04-09 18:51:08 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #14)
> We haven't been enforcing this at all for the graphics module at all. The
> current list of peers in that document is considerably out of date - Though
> it may be technically correct since new peers haven't been discussed at all
> to my knowledge.
> 
> I think this can probably be relanded, considering that the majority of
> recent gfx work would fall under the same classification.

Yeah, the majority of recent WebGL work is me and bjacob reviewing eachothers' patches. Should we just add the lot of us as peers, so as to make the documentation match the reality?
Comment 17 Benoit Jacob [:bjacob] (mostly away) 2012-04-09 19:22:05 PDT
This 'peer' thing just gets in the way and I had forgotten it existed before this reminded me about it. Please just reland?
Comment 18 George Wright (:gw280) (:gwright) 2012-04-09 19:26:00 PDT
I agree with bjacob. It seems that the gfx module doesn't follow the normal review rules, for one reason or another..
Comment 19 Benoit Jacob [:bjacob] (mostly away) 2012-04-09 19:35:14 PDT
In fact, the reality is that different people are better reviewers for different files. Even if someone is relatively new, they might still be the best reviewer for a part of the codebase where they have been the most active since they started.
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-04-09 19:40:55 PDT
I'll reland this tomorrow if nobody else beats me to it.
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-04-11 05:03:10 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f64fb9a403e6

For great justice.
Comment 23 :Ehsan Akhgari (out sick) 2012-04-12 10:11:59 PDT
https://hg.mozilla.org/mozilla-central/rev/f64fb9a403e6

Note You need to log in before you can comment on or make changes to this bug.