Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use MutableByteArray as the primitive for the ring buffer #2761

Merged
merged 9 commits into from
Jul 9, 2024
Merged

Use MutableByteArray as the primitive for the ring buffer #2761

merged 9 commits into from
Jul 9, 2024

Conversation

adithyaov
Copy link
Member

This looks suspiciously good.

Generating reports for Data.Ring.Unboxed...
Data.Ring.Unboxed(cpuTime)
Benchmark                                      default(0)(ms) default(1) - default(0)(%)
---------------------------------------------- -------------- --------------------------
All.Data.Ring.Unboxed/o-1-space.unsafeEqArray            5.55                    -566.29
All.Data.Ring.Unboxed/o-1-space.unsafeEqArrayN           1.01                    -960.54

Data.Ring.Unboxed(Allocated)
Benchmark                                      default(0)(MiB) default(1) - default(0)(%)
---------------------------------------------- --------------- --------------------------
All.Data.Ring.Unboxed/o-1-space.unsafeEqArray            24.33                  -Infinity
All.Data.Ring.Unboxed/o-1-space.unsafeEqArrayN            1.60                  -Infinity

Data.Ring.Unboxed(maxrss)
Benchmark                                      default(0)(MiB) default(1) - default(0)(%)
---------------------------------------------- --------------- --------------------------
All.Data.Ring.Unboxed/o-1-space.unsafeEqArray            15.00                       0.00
All.Data.Ring.Unboxed/o-1-space.unsafeEqArrayN           15.00                       0.00

Generating reports for Data.Array...
Data.Array(cpuTime)
Benchmark                                                        default(0)(μs) default(1) - default(0)(%)
---------------------------------------------------------------- -------------- --------------------------
All.Data.Array/o-n-space.elimination.writeLastN.Max                     1894.99                      -4.93
All.Data.Array/o-1-space.elimination.writeLastN.10                        88.07                      -6.87
All.Data.Array/o-1-space.elimination.writeLastN.1                         94.66                     -39.35

Data.Array(Allocated)
Benchmark                                                        default(0)(Bytes) default(1) - default(0)(%)
---------------------------------------------------------------- ----------------- --------------------------
All.Data.Array/o-1-space.elimination.writeLastN.1                             0.00                        NaN
All.Data.Array/o-1-space.elimination.writeLastN.10                            0.00                        NaN
All.Data.Array/o-n-space.elimination.writeLastN.Max                    15233357.00                     -12.77

Data.Array(maxrss)
Benchmark                                                        default(0)(MiB) default(1) - default(0)(%)
---------------------------------------------------------------- --------------- --------------------------
All.Data.Array/o-n-space.elimination.writeLastN.Max                        17.00                       0.00
All.Data.Array/o-1-space.elimination.writeLastN.10                         15.00                       0.00
All.Data.Array/o-1-space.elimination.writeLastN.1                          15.00                       0.00

This PR supersedes:

@adithyaov
Copy link
Member Author

I need to add some more sanity tests before merging this.

@adithyaov adithyaov force-pushed the enhance-ring-buffer branch 2 times, most recently from 3d2b44f to ad8ebce Compare July 5, 2024 05:51
harendra-kumar
harendra-kumar requested changes Jul 8, 2024
View reviewed changes
Copy link
Member

@harendra-kumar harendra-kumar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the changelog, packdiff reports some constraint changes.

core/src/Streamly/Internal/Data/Fold/Combinators.hs Outdated
@@ -1676,7 +1675,7 @@ takeEndBySeq patArr (Fold fstep finitial fextract ffinal) =
res <- fstep s x
case res of
Partial s1 -> do
old <- liftIO $ peek rh
(old :: a) <- liftIO $ PEEK_ELEM(a, rh, (Ring.ringContents rb))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this passing hlint? it has redundant brackets.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any redundant brackets.
Are the brackets here (old :: a) redundant?
If so, need to check hlint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe because PEEK_ELEM is a macro, my hls was showing a redundant bracket around ringContents rb.

core/src/Streamly/Internal/Data/Ring.hs Outdated Show resolved Hide resolved
core/src/Streamly/Internal/Data/Ring.hs Outdated Show resolved Hide resolved
core/src/Streamly/Internal/Data/Ring.hs Show resolved Hide resolved
core/src/Streamly/Internal/Data/Ring.hs Show resolved Hide resolved
core/src/Streamly/Internal/Data/Ring.hs Show resolved Hide resolved
core/src/Streamly/Internal/Data/Ring.hs Outdated Show resolved Hide resolved
core/src/Streamly/Internal/Data/Ring.hs Outdated Show resolved Hide resolved
core/src/Streamly/Internal/Data/Ring.hs Outdated Show resolved Hide resolved
core/src/Streamly/Internal/Data/Ring.hs Outdated Show resolved Hide resolved
harendra-kumar
harendra-kumar reviewed Jul 8, 2024
View reviewed changes
core/src/Streamly/Internal/Data/Ring.hs
@@ -137,7 +147,7 @@ advance rb ringHead =
-- array.
{-# INLINE moveBy #-}
moveBy :: Int -> Ring a -> Int -> Int
moveBy by rb ringHead = (ringHead + by) `mod` ringCapacity rb
moveBy by rb ringHead = (ringHead + by) `mod1` ringCapacity rb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually we should not be moving it by more than the ring size. But we have to handle the case if someone does that. Anyway, it seems this function is not used anywhere.

@adithyaov adithyaov merged commit d23a5d8 into master Jul 9, 2024
14 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harendra-kumar harendra-kumar harendra-kumar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adithyaov @harendra-kumar