Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Use the more efficient unsafeShiftR instead of shiftR when base >= 4.5 #71

Closed
wants to merge 1 commit into from

3 participants

@basvandijk
Collaborator

I may commit this myself when I have done some benchmarks. But I think this will improve performance a bit.

@basvandijk basvandijk was assigned
@bos
Owner
bos commented

In my experience with recent versions of GHC, the compiler usually spots shifts by small constants and does an unchecked shift. @tibbe, does this match what you've seen?

In any case, look at the generated Core before you worry about benchmarking. You may find that your change makes no difference at all.

@tibbe

Like @bos said, I'd start by looking at the core. IIRC if the shift amount is constant, GHC will generate good code. What shiftR does, and what is sometimes bad, is that it introduces a branch like so:

-- | Shift the argument right (signed) by the specified number of bits
-- (which must be non-negative).
iShiftRA# :: Int# -> Int# -> Int#
a `iShiftRA#` b | b >=# WORD_SIZE_IN_BITS# = if a <# 0# then (-1#) else 0#
                | otherwise                = a `uncheckedIShiftRA#` b

The reason it does so is that the underlying assembly instructions for shifts are undefined for shift amounts larger than the WORD_SIZE_IN_BITS and whoever designed shiftR thought safety was more important than speed. With inlining shiftR should be equivalent to unsafeShiftR, if the shift amount is known.

@bos bos closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 10 additions and 2 deletions.
  1. +10 −2 Data/Aeson/Types/Generic.hs
View
12 Data/Aeson/Types/Generic.hs
@@ -1,4 +1,4 @@
-{-# LANGUAGE DefaultSignatures, EmptyDataDecls, FlexibleInstances,
+{-# LANGUAGE CPP, DefaultSignatures, EmptyDataDecls, FlexibleInstances,
FunctionalDependencies, KindSignatures, OverlappingInstances,
ScopedTypeVariables, TypeOperators, UndecidableInstances, ViewPatterns #-}
{-# OPTIONS_GHC -fno-warn-orphans #-}
@@ -21,7 +21,7 @@ import Control.Applicative ((<*>), (<$>), (<|>), pure)
import Control.Monad.ST (ST)
import Data.Aeson.Types.Class
import Data.Aeson.Types.Internal
-import Data.Bits (shiftR)
+import Data.Bits
import Data.DList (DList, toList)
import Data.Monoid (mappend)
import Data.Text (pack, unpack)
@@ -106,7 +106,11 @@ instance (GProductToValues a, GProductToValues b) => GProductToValues (a :*: b)
gProductToValues mv ix len (a :*: b) = do gProductToValues mv ix lenL a
gProductToValues mv ixR lenR b
where
+#if MIN_VERSION_base(4,5,0)
+ lenL = len `unsafeShiftR` 1
+#else
lenL = len `shiftR` 1
+#endif
ixR = ix + lenL
lenR = len - lenL
{-# INLINE gProductToValues #-}
@@ -235,7 +239,11 @@ instance (GFromProduct a, GFromProduct b) => GFromProduct (a :*: b) where
gParseProduct arr ix len = (:*:) <$> gParseProduct arr ix lenL
<*> gParseProduct arr ixR lenR
where
+#if MIN_VERSION_base(4,5,0)
+ lenL = len `unsafeShiftR` 1
+#else
lenL = len `shiftR` 1
+#endif
ixR = ix + lenL
lenR = len - lenL
{-# INLINE gParseProduct #-}
Something went wrong with that request. Please try again.