737239 - Use bit twiddling for gfxUtils::NextPowerOfTwo on non-arm builds

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Nick Cameron [:nrc]]

Attachment: patch

Comment 2

[George Wright (:gw280) (needinfo me!)]

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]]

Attachment: 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) (needinfo me!)]

Comment on attachment 607671 [details] [diff] [review]
updated patch

lgtm

Comment 5

[George Wright (:gw280) (needinfo me!)]

(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]]

Attachment: updated patch

Fixed indent, undeleted copyright notice.

Comment 7

[Nick Cameron [:nrc]]

Successfully navigated push to Try: https://tbpl.mozilla.org/?tree=Try&rev=43826350ded4

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

[Kelsey Gilbert [:jgilbert]]

(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) (needinfo me!)]

(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]]

errr...so why are patches being checked in on your r+?

Comment 12

[Nick Cameron [:nrc]]

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]]

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

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

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

[Kelsey Gilbert [:jgilbert]]

(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)]

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) (needinfo me!)]

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

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]]

I'll reland this tomorrow if nobody else beats me to it.

Comment 21

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/e54c650e8748
https://hg.mozilla.org/mozilla-central/rev/939ec96b6b38

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f64fb9a403e6

For great justice.

Comment 23

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/f64fb9a403e6