Stop using prbit.h

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla28
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 828358 [details] [diff] [review]
Part 1: Kill most uses of prbit.h
Attachment #828358 - Flags: review?(ehsan)
(Assignee)

Comment 1

5 years ago
Created attachment 828359 [details] [diff] [review]
Part 2: Implement RotateLeft/RotateRight in MFBT, and kill uses of NSPR equivalents
Attachment #828359 - Flags: review?(jwalden+bmo)

Comment 2

5 years ago
Comment on attachment 828358 [details] [diff] [review]
Part 1: Kill most uses of prbit.h

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

::: xpcom/glue/pldhash.cpp
@@ +59,5 @@
>  
>  #else
>  
> +#define INCREMENT_RECURSION_LEVEL(table_)   do { } while(0)  
> +#define DECREMENT_RECURSION_LEVEL(table_)   do { } while(0)  

Nit: please remove these trailing whitespaces (and the above ones too)
Attachment #828358 - Flags: review?(ehsan) → review+

Comment 3

5 years ago
Comment on attachment 828359 [details] [diff] [review]
Part 2: Implement RotateLeft/RotateRight in MFBT, and kill uses of NSPR equivalents

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

There's also a JS_ROTATE32_LEFT in js/public/Utility.h, because of course there'd be.  Could you write a followup patch to kill that as well?  That also seems a good time to potentially fold in the optimizations in that macro -- if they still apply at all (perhaps MSVC has picked up the ability to detect and peephole-optimize rotate32 shift/or operations since it was written).  But this patch is fine as written, to be sure.

::: mfbt/MathAlgorithms.h
@@ +451,5 @@
>    return size_t(1) << CeilingLog2(x);
>  }
>  
> +/**
> + * Rotates the given value left by the amount of the shift width.

Maybe "Rotates the bits of" to be slightly clearer about what's happening.

@@ +457,5 @@
> +template<typename T>
> +inline T
> +RotateLeft(const T t, uint_fast8_t shift)
> +{
> +  static_assert(IsUnsigned<T>::value, "Rotates require unsigned values");

MOZ_ASSERT(shift < sizeof(T) * CHAR_BIT, "shifting too much!");

@@ +458,5 @@
> +inline T
> +RotateLeft(const T t, uint_fast8_t shift)
> +{
> +  static_assert(IsUnsigned<T>::value, "Rotates require unsigned values");
> +  return (t << shift) | (t >> (sizeof(T) * 8 - shift));

s/8/CHAR_BIT/

@@ +469,5 @@
> +inline T
> +RotateRight(const T t, uint_fast8_t shift)
> +{
> +  static_assert(IsUnsigned<T>::value, "Rotates require unsigned values");
> +  return (t >> shift) | (t << (sizeof(T) * 8 - shift));

...and all the same nits as on the other method, applied to this one.
Attachment #828359 - Flags: review?(jwalden+bmo) → review+
prbits.h already has an optimization for MSVC.
https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prbit.h#143
IMO we should copy them to MFBT.
(Assignee)

Comment 5

5 years ago
(In reply to Masatoshi Kimura [:emk] from comment #4)
> prbits.h already has an optimization for MSVC.
> https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prbit.h#143
> IMO we should copy them to MFBT.

I asked someone to test on IRC, and MSVC 2010 is apparently capable of reducing (x << a) | (x >> (32 - a)) into a rotate-left instruction. gcc and clang are both already capable of this, so there's no need to resort to #ifdef trickery here. And I prefer avoiding #ifdef's whenever I can get away with it.
(Assignee)

Updated

5 years ago
Blocks: 937952
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f28e77e0a20
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3792d389bc

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> There's also a JS_ROTATE32_LEFT in js/public/Utility.h, because of course
> there'd be.  Could you write a followup patch to kill that as well?  That
> also seems a good time to potentially fold in the optimizations in that
> macro -- if they still apply at all (perhaps MSVC has picked up the ability
> to detect and peephole-optimize rotate32 shift/or operations since it was
> written).  But this patch is fine as written, to be sure.

Filed as bug 937952.
https://hg.mozilla.org/mozilla-central/rev/6f28e77e0a20
https://hg.mozilla.org/mozilla-central/rev/7c3792d389bc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.